View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002369 | FSSCP | tables | public | 2011-01-04 15:00 | 2011-11-29 06:04 |
| Reporter | Fury | Assigned To | Goober5000 | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | Antipodes 8 | ||||
| Summary | 0002369: Setting $SBank Capacity in shp.tbm also requires $Default SBanks | ||||
| Description | If omitted, debug gives following error f-shp.tbm(line 41:Error: Required token = [#End], [$Subsystem:], [$Name], or [$Template], found [$SBank Capacity: ( 40, 40 )]. | ||||
| Tags | No tags attached. | ||||
|
|
Reverse is also true, omitting $SBank Capacity when $Default SBanks it set also produces debug error. f-shp.tbm(line 42:Error: Missing required token: [$SBank Capacity:]. Found [$Shields: 480] |
|
|
I've made a patch which removes $SBank Capacity's dependency on $Default Banks, so you can now include the former regardless of whether the latter is present. The reverse, however, I think seems to make sense, so in my (admittedly limited) view, I vote it should be kept as is. This does need coder review though. EDIT: Patch is here: http://pastebin.com/ExSzKerF |
|
|
CommanderDJ - the patch looks like it'll work fine, however I'm not so keen on copy/paste of code. What about using a function to do the work of parsing "$SBank Capacity:"? i.e. ... required_string("$SBank Capacity:"); function_call(); ... else if(optional_string("$SBank Capacity:")) function_call(); ... |
|
|
I went one better and removed the need for duplicate code. I've also removed the dependency both ways now, so you can have either one of $Default SBanks or $SBank Capacity without the other. In the process I found something that would have actually prevented the previous patch from working - the code determined the number of secondary banks on a ship from its default banks. If the number of capacities didn't match up, it threw a warning. Since the two are no longer dependent I've done it this way: if $Default SBanks is specified, the code uses that to determine the number of secondary banks. If $SBank Capacity is also specified, it double checks that the two match up as before. If Capacity is specified but not Defaults, it uses the number of Capacities as the number of banks (keep in mind any discrepancies between model and table get picked up on mission load - during game startup it only checks for mismatches within the tables). I think this should work for everyone, but if there are any other suggestions I'm happy to work on them. EDIT: new patch is here: http://pastebin.com/vv9Q2cqe |
|
|
The patch appears to make $Default SBanks optional when being parsed in a .tbl as well? How does FSO/FRED handle a ship that has no SBanks specfied at all? |
|
|
FSO/FRED sets the defaults to the first secondary available in weapons.tbl if none are specified. I have tested it with no capacity and nothing broke, but I'm not sure exactly what it does. I'll do some more testing later today and post here with any findings. |
|
|
How is this even a bug? A ship *always* needs default SBanks so that FRED knows what to assign the ship when it's created, or so that ship-create knows what to put in the missile bays. If you're saying the TBM requires default SBanks when they were already specified in the table, then that's legitimate. But the suggested patch doesn't appear to capture all scenarios. What if, as asked on the forum, default SBanks were never specified in either the tbl or tbm? It looks like at least two arrays would go uninitialized. |
|
|
Also, a while back I think I eliminated the need for a ship to have SBanks at all. TIE Fighters in FotG seem to be working fine without them. Hope it doesn't affect that adversely. |
|
|
Currently Default Sbanks is an optional setting. As CommanderDJ says the game is capable of figuring out what it should use for the SBanks in that situation. If you've made Default SBanks optional then you need to make sure that nothing else screws up as a result. Which is what this fix is supposed to be doing. The only other alternative would be to set Default SBanks back to required. |
|
|
Karajorma's right. Default SBanks was optional beforehand, the game simply assigns the first secondary in weapons.tbl as the defaults if it isn't specified. I'm currently investigating what it does for capacity, but I do recall nothing breaking. I'll post here when I have more info. |
|
|
If the ship doesn't have any SBanks at all, then it shouldn't have any SBank capacities either. So the TIE Fighter example doesn't seem relevant here. If you specify Defaults but don't specify Capacities, then what capacities should be used by default? Zero? If you specify Capacities but don't specify Defaults, that just strikes me as being lazy. You can use the first secondary listed in the table, but that's almost always the Rockeye and may not be suitable for all ships. For those reasons it's pretty self-evident that Defaults and Capacities should always be specified together. Even if it may be possible to make them work independently, I don't think it's a good idea. In fact, I originally thought that this patch would cause problems due to not initializing the sip->secondary_bank_weapons array in certain code branches. Fortunately, the ship_set() method initializes everything to 0, so the absence of explicit initialization here does not harm anything. Even so, a comment should probably be added explaining this. There's also the issue that if the Capacity but not the Defaults is specified in a modular table, and the number of Capacity banks in the TBM happens to be different from the number of Default banks in the original ships.tbl, the parser will silently change the number of banks without showing a warning like it should. This is because multiple TBMs require multiple invocations of the parse_ship_values function, and the sbank_defaults_specified variable does not persist from invocation to invocation. |
|
|
Sorry to bump this, but are we leaving it as is then? If so, can this ticket be closed? |
|
|
Well, after further consideration, I realized that what I was actually concerned about was not having both lines specified before parsing *completed*, as opposed to not having both lines consecutively in the same file. So I instead implemented a fix where the parser was made a little more robust and a little more forgiving. It now complains if Default Banks and Bank Capacities get out of sync, but it doesn't complain if you modify them one-at-a-time in a modular table. Fixed in r8054. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2011-01-04 15:00 | Fury | New Issue | |
| 2011-01-04 15:02 | Fury | Note Added: 0012587 | |
| 2011-10-29 12:58 | CommanderDJ | Note Added: 0012909 | |
| 2011-10-29 12:59 | CommanderDJ | Note Edited: 0012909 | |
| 2011-10-29 13:00 | CommanderDJ | Note Edited: 0012909 | |
| 2011-10-30 03:38 | niffiwan | Note Added: 0012910 | |
| 2011-10-30 06:18 | CommanderDJ | Note Added: 0012911 | |
| 2011-10-30 06:19 | CommanderDJ | Note Edited: 0012911 | |
| 2011-10-30 09:40 | CommanderDJ | Note Edited: 0012911 | |
| 2011-11-04 01:52 | iss_mneur | Note Added: 0012919 | |
| 2011-11-04 01:52 | iss_mneur | Status | new => acknowledged |
| 2011-11-04 02:14 | CommanderDJ | Note Added: 0012920 | |
| 2011-11-04 15:28 | Goober5000 | Status | acknowledged => assigned |
| 2011-11-04 15:28 | Goober5000 | Assigned To | => Goober5000 |
| 2011-11-04 15:31 | Goober5000 | Note Added: 0012922 | |
| 2011-11-04 15:43 | chief1983 | Note Added: 0012923 | |
| 2011-11-05 01:20 | karajorma | Note Added: 0012926 | |
| 2011-11-05 03:00 | CommanderDJ | Note Added: 0012927 | |
| 2011-11-07 05:01 | Goober5000 | Note Added: 0012935 | |
| 2011-11-28 11:00 | CommanderDJ | Note Added: 0012985 | |
| 2011-11-29 06:04 | Goober5000 | Note Added: 0012986 | |
| 2011-11-29 06:04 | Goober5000 | Status | assigned => resolved |
| 2011-11-29 06:04 | Goober5000 | Resolution | open => fixed |
