View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002513 | FSSCP | models | public | 2011-10-03 10:32 | 2012-12-18 21:14 |
Reporter | niffiwan | Assigned To | Admiral MS | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.13 | ||||
Summary | 0002513: Ships that share a model also share subsystem flags | ||||
Description | This was noticed when testing mantis 2470. In the example mission from bigchunk1 there are two ships which share the Fenris model (cruiser01.pof). One of the ships is tabled to have all turrets and subsystems flagged as "untargetable". Ignoring the turrets for now (that's what 2470 is about), you'll note that the subsystems on the "untargetable" Fenris can be targeted. The 2nd Fenris is using all the subsystem data (including flags) from the 1st Fenris. | ||||
Additional Information | Stacktrace of where the problem occurs: (gdb) backtrace #0 model_copy_subsystems (n_subsystems=15, d_sp=0x40b2400, s_sp=0x23cb500) at model/modelread.cpp:398 0000001 0x0000000000765975 in ship_copy_subsystem_fixup (sip=0x1c12a40) at ship/ship.cpp:5111 0000002 0x000000000076f890 in ship_create (orient=0x7fffe41bd0bc, pos=0x7fffe41bd0b0, ship_type=113, ship_name=0x7fffe41bd080 "Notgt Fen 2") at ship/ship.cpp:8449 0000003 0x00000000005c9947 in parse_create_object_sub (p_objp=0x7fffe41bd080) at mission/missionparse.cpp:1785 0000004 0x00000000005c9862 in parse_create_object (pobjp=0x7fffe41bd080) at mission/missionparse.cpp:1748 0000005 0x00000000005ce071 in mission_parse_maybe_create_parse_object (pobjp=0x7fffe41bd080) at mission/missionparse.cpp:3234 0000006 0x00000000005d0e08 in post_process_ships_wings () at mission/missionparse.cpp:4481 0000007 0x00000000005d31c5 in post_process_mission () at mission/missionparse.cpp:5405 0000008 0x00000000005d30e7 in parse_mission (pm=0xfd4e60, flags=0) at mission/missionparse.cpp:5385 0000009 0x00000000005d4199 in parse_main (mission_name=0x7fffffffdee0 "TestBC101.fs2", flags=0) at mission/missionparse.cpp:5764 0000010 0x00000000005bb12f in mission_load (filename_ext=0xbf1540 "TestBC101") at mission/missionload.cpp:107 #11 0x000000000040cf22 in game_start_mission () at freespace2/freespace.cpp:1453 0000012 0x0000000000416791 in game_enter_state (old_state=20, new_state=52) at freespace2/freespace.cpp:5968 0000013 0x00000000004b7614 in gameseq_set_state (new_state=52, override=0) at gamesequence/gamesequence.cpp:282 0000014 0x0000000000415688 in game_process_event (current_state=20, event=1) at freespace2/freespace.cpp:5150 0000015 0x00000000004b7b0a in gameseq_process_events () at gamesequence/gamesequence.cpp:397 0000016 0x0000000000417f47 in game_main (cmdline=0x229aa50 "") at freespace2/freespace.cpp:7086 0000017 0x000000000041811c in main (argc=1, argv=0x7fffffffe318) at freespace2/freespace.cpp:7220 (gdb) | ||||
Tags | No tags attached. | ||||
2011-10-03 10:32
|
|
|
Uploaded a patch with a potential solution. I'm not really sure if this is a good idea though, it may cause other bugs I'm not aware of. |
|
It is potentially good, but I'm not exactly sure that this is best approach either. Unfortunately, I can't exactly put my finger on what bothers me about this problem. Lets see what comes of somebody else taking a look at this before calling it either way. |
|
It seems my patch is just as bad as the original bug. The problem is that flags set in the model will always be read after table parsed flags (there seems to be no way to do it the other way round) and I can't find a way to get them without table flags. Alternative would be checking only the 5 model flags in model_copy_subsystems like this: if ( source->flags & MSS_FLAG_CREWPOINT ) dest->flags |= MSS_FLAG_CREWPOINT; This on the other hand is likely to cause issues if new flags get added to the model files and no one adds them to model_copy_subsystems. |
|
My recollection was that the lookup for the ship flags was done via the model number, so when you had a shared model the flags were retrieved from the wrong ship (i.e. the 1st model found). But given that I'm going on my 14 month old memories, I'll have another look at the code over the next few days so I can give some better feedback. |
|
I think I understand this all better now :) I haven't checked how the model flags are read from the .pof yet and I probably won't get a chance for a few more days, so I thought I'd throw this idea out there now. Maybe you could create a new #define, something like SIF_MASK, except for the model based MSS flags. That way the relevant flags that we need to copy are defined right next to all the other flags, so if any new flags are added there's a reminder in the same place to get the coder thinking about if they're model flags or not. |
|
Made a new patch that uses a #define directly below the flag definitions as you suggested. |
|
modelflags.patch (1,271 bytes)
Index: code/model/model.h =================================================================== --- code/model/model.h (revision 9449) +++ code/model/model.h (working copy) @@ -141,6 +141,10 @@ #define NUM_SUBSYSTEM_FLAGS 33 +// all subsys flags set in model file, used to copy only these flags for different table entries using the same model +#define MSS_MODEL_FLAG (MSS_FLAG_CREWPOINT | MSS_FLAG_ROTATES | MSS_FLAG_TRIGGERED | MSS_FLAG_ARTILLERY | MSS_FLAG_STEPPED_ROTATE) +#define MSS_MODEL_FLAG2 0 + // definition of stepped rotation struct typedef struct stepped_rotation { int num_steps; // number of steps in complete revolution Index: code/model/modelread.cpp =================================================================== --- code/model/modelread.cpp (revision 9449) +++ code/model/modelread.cpp (working copy) @@ -404,7 +404,8 @@ for ( j = 0; j < n_subsystems; j++ ) { dest = &d_sp[j]; if ( !subsystem_stricmp( source->subobj_name, dest->subobj_name) ) { - dest->flags = source->flags; + dest->flags |= (source->flags & MSS_MODEL_FLAG); + dest->flags2 |= (source->flags2 & MSS_MODEL_FLAG2); dest->subobj_num = source->subobj_num; dest->model_num = source->model_num; dest->pnt = source->pnt; |
|
The test mission works as expected. Pending a positive code review, i'd say this is fixed. |
|
patch looks good to me, if you've got commit access go ahead and commit. Otherwise I'll do it tonight. |
|
Fix committed to trunk@9451. |
Date Modified | Username | Field | Change |
---|---|---|---|
2011-10-03 10:32 | niffiwan | New Issue | |
2011-10-03 10:32 | niffiwan | File Added: TurretUntargetableTest.zip | |
2012-12-09 00:19 | Admiral MS | File Added: subsys_flags.patch | |
2012-12-09 00:21 | Admiral MS | Note Added: 0014363 | |
2012-12-09 00:27 | Admiral MS | Note Edited: 0014363 | |
2012-12-09 00:43 | Admiral MS | Note Edited: 0014363 | |
2012-12-09 03:43 | Zacam | Assigned To | => niffiwan |
2012-12-09 03:43 | Zacam | Status | new => code review |
2012-12-09 05:35 | Zacam | Note Added: 0014375 | |
2012-12-09 11:58 | Admiral MS | Note Added: 0014384 | |
2012-12-10 02:17 | niffiwan | Note Added: 0014398 | |
2012-12-14 11:26 | niffiwan | Note Added: 0014452 | |
2012-12-18 20:28 | Admiral MS | Note Added: 0014524 | |
2012-12-18 20:28 | Admiral MS | File Deleted: subsys_flags.patch | |
2012-12-18 20:28 | Admiral MS | File Added: modelflags.patch | |
2012-12-18 20:40 | MjnMixael | Note Added: 0014526 | |
2012-12-18 21:08 | niffiwan | Note Added: 0014527 | |
2012-12-18 21:08 | niffiwan | Assigned To | niffiwan => Admiral MS |
2012-12-18 21:08 | niffiwan | Status | code review => assigned |
2012-12-18 21:08 | niffiwan | Status | assigned => code review |
2012-12-18 21:14 | The_E | Changeset attached | => fs2open trunk r9451 |
2012-12-18 21:14 | The_E | Note Added: 0014528 | |
2012-12-18 21:14 | The_E | Status | code review => resolved |
2012-12-18 21:14 | The_E | Resolution | open => fixed |