View Issue Details

IDProjectCategoryView StatusLast Update
0002513FSSCPmodelspublic2012-12-18 21:14
Reporterniffiwan Assigned ToAdmiral MS  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.13 
Summary0002513: Ships that share a model also share subsystem flags
DescriptionThis 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 InformationStacktrace 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)
TagsNo tags attached.

Activities

2011-10-03 10:32

 

Admiral MS

2012-12-09 00:21

developer   ~0014363

Last edited: 2012-12-09 00:43

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.

Zacam

2012-12-09 05:35

administrator   ~0014375

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.

Admiral MS

2012-12-09 11:58

developer   ~0014384

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.

niffiwan

2012-12-10 02:17

developer   ~0014398

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.

niffiwan

2012-12-14 11:26

developer   ~0014452

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.

Admiral MS

2012-12-18 20:28

developer   ~0014524

Made a new patch that uses a #define directly below the flag definitions as you suggested.

Admiral MS

2012-12-18 20:28

developer  

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;
modelflags.patch (1,271 bytes)   

MjnMixael

2012-12-18 20:40

manager   ~0014526

The test mission works as expected. Pending a positive code review, i'd say this is fixed.

niffiwan

2012-12-18 21:08

developer   ~0014527

patch looks good to me, if you've got commit access go ahead and commit. Otherwise I'll do it tonight.

The_E

2012-12-18 21:14

administrator   ~0014528

Fix committed to trunk@9451.

Related Changesets

fs2open: trunk r9451

2012-12-18 16:46

The_E


Ported: N/A

Details Diff
Fix for Mantis 2513 by Admiral MS: Fixes issues with two ship classes sharing model data. Affected Issues
0002513
mod - /trunk/fs2_open/code/model/model.h Diff File
mod - /trunk/fs2_open/code/model/modelread.cpp Diff File

Issue History

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