View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
---|---|---|---|---|---|---|---|---|---|
0002159 | FSSCP | tables | public | 2010-03-22 13:49 | 2010-09-14 20:54 | ||||
Reporter | Genghis | ||||||||
Assigned To | karajorma | ||||||||
Priority | normal | Severity | minor | Reproducibility | always | ||||
Status | resolved | Resolution | fixed | ||||||
Product Version | 3.6.12 RC1 | ||||||||
Target Version | Fixed in Version | 3.6.13 | |||||||
Summary | 0002159: Extra error-checking is needed for certain table values. | ||||||||
Description | Certain table values are used as look-ups into other tables, or are otherwise important and need to be meaningful. Many of them have validation checks that trigger warnings or errors if they fail, but some do not and fail silently. For example: When a ship subsystem has an unknown armor type name, or when the IFF names are unknown. The attached patch extra_error_checking.diff includes additional warnings for all of these cases that I found. | ||||||||
Tags | No tags attached. | ||||||||
Attached Files |
|
![]() |
|
FUBAR-BDHR (developer) 2010-04-02 04:37 |
Getting the same issue as with 0002151 but with subsystems overridden by .tbm. |
Genghis (developer) 2010-04-04 11:26 |
Is this an impediment to releasing it? Any values which were being checked before this patch (e.g. "$Damage Lightning Type:") also exhibit this behaviour, raising the warning on the bad table entry even though a good override is coming later in a .tbm. Are there mods out there which depend on this behaviour? Reason I ask is because it would be a bitch to fix since the parser is totally unaware when it encounters the bad table entry that there's going to be a good override coming later. I'd have to move -all- the checking to the end, or rearrange the parser somehow, and that's a BigProject(tm). |
chief1983 (administrator) 2010-04-04 12:45 |
Currently the custom hud gauges table does virtually no error checking or reporting, but it's already being rewritten by Swifty and the new incarnation should have some level of that. |
FUBAR-BDHR (developer) 2010-04-04 16:26 |
Well the biggest problem I see is that the usual way to make a patch for errors found with new builds is using a .tbm with +nocreate and just fix what's broken. When you do that with the new error checks you still get the same error messages you just fixed. Also just because you are using the .tbl doesn't mean it's in your directory or your asset. You may be making a mod XYZ of mod ABC. ABC contains armor.tbl and ships.tbl with say armor types Armor1, Armor2, Armor3, etc that is assigned to ships in ship.tbl. You are using that ship.tbl from ABC via mod.ini but don't like the armor setup so you make your own armor.tbl and use newarmor-shp.tbm in ABC. Each ship in newarmor.shp.tbl has a +nocreate entry with armor type set to better_armor1, 2, 3, etc. These armor types are perfectly valid but when ships.tbl from ABC parses it sees the old ones and throws an error for every ship with an armor entry. Not a good thing. Probably why no error checking was ever done for this stuff. |
Genghis (developer) 2010-04-05 22:15 Last edited: 2010-04-05 22:16 |
I don't see a way around this without one of two options: 1) a fairly intrusive rewrite of the parser. 2) No errors. I've actually had some ideas about how I'd like to do the parsing in a more data-driven way compared to the current method (very hard-coded). It would also make the error checking happen as a second pass after parsing is finished, and so this table/.tbm issue would disappear. I'll prototype-up my thoughts and make something that replaces the loader for ships, and see where that takes me. Should be fun, will probably take me a couple of weeks. |
FUBAR-BDHR (developer) 2010-04-05 22:32 |
Well there is an option 3. Make these checks only occur when a dev_tools flag is on. They are all good to know when doing development. This could also be a temp solution until a better solution is available. Personally I like the second pass idea for standard detection of problems like this. Of course it would still be a good idea to allow a check all option like above. If you can clean up every single bad piece of data in a TC or mod you should and knowing they exist is the first step. |
Genghis (developer) 2010-04-06 09:42 |
Option 3 is actually pretty good, it's something I could do right away. It would be a good stop-gap at least. |
chief1983 (administrator) 2010-04-06 11:00 |
Any intrusive rewrite would definitely be a post-3.6.12 thing, but I'm definitely in support of it. Just need to keep tabs on the rest of the team to make sure it doesn't step on anyone's toes. FUBAR, isn't that essentially what debug builds are? Dev_tools? Actually that's not entirely true of course, and I've been pushing for separating mod dev tools from actual code debugging. I believe that certain warnings should be available within Release builds via a flag, so as not to hurt runtime performance so strongly but still offer mods help catching most asset problems. Not really related to this, but that's how I'd like to see a flag like that used. |
FUBAR-BDHR (developer) 2010-04-06 12:29 |
Well I haven't actually tried this patch in a release build but what worried me was the window that pops up that say something like this mod has xx errors right before pilot select. I was getting that with these errors in debug and I believe you get that in release as well. Anyway this may be one case where you want the ability to disable the checking since it does give warnings that currently aren't actual errors. If you don't it will be the whole TBP null MOI issue again with no one wanting to check errors on mods because there are just too many error messages to bypass in the debugger. |
Genghis (developer) 2010-04-06 13:45 |
My plan was to have a "verbosity" option with different levels: level 0 = no warnings, asserts, Int3s, if there's a fatal problem it just crashes. level 1 (release build default) = only warn about potentially fatal problems, i.e. Asserts, Int3s, etc. level 2 (debug build default) = Warnings for non-fatal but still "show-stopping" problems. level 3 = maximum chatter. all warnings. It won't affect much, there'll be "default" versions of the Warning, Int3, etc functions that behave exactly as before, and then overloaded versions where you can specify a verbose level. I'll then go through my patch and drop all the new warnings to level 3. |
FUBAR-BDHR (developer) 2010-04-06 14:26 |
Scrap level 0 and it's a good plan. Coupled with the current (and probably expanded) debug_filter.cfg style system it would be a real step forward in finding bugs and preventing crashes. |
chief1983 (administrator) 2010-04-06 14:53 |
Actually, currently release builds do ignore asserts and such. Do we want to have them be more verbose than they currently are you mean? |
FUBAR-BDHR (developer) 2010-04-06 15:08 |
I though current release builds had some error checking now and show a warning about number of errors but let you continue. I wouldn't want to remove that. |
chief1983 (administrator) 2010-04-06 15:12 |
That's all though. They skip over asserts entirely I believe. |
Genghis (developer) 2010-04-09 00:58 |
Alright that warning/logging/error system is a bit of a mess, so I did the minimum change necessary rather than risk a refactor with verbose levels. I added a new cmdline option "-extra_warn", and a new function WarningEx. When extra_warn is on, WarningEx prints a regular warning (with callstack, etc). When it's off, the function does nothing, doesn't even increment the global warnings count. I then modified my patch to use WarningEx everywhere instead of Warning. That oughta do it. Incidentally, did you know there was a "-no_warn" option and a "-noparseerrors" one? |
chief1983 (administrator) 2010-04-09 10:49 |
Yup. People have abused -no_warn as a means to bypass any warning using debug builds, which has just been silly because that's the whole point of using them. |
karajorma (administrator) 2010-05-02 01:43 |
Actually most of the abuse was centred around bypassing warnings in the release builds back when they used to show warnings. Warnings were then changed to only show up in debug builds and the flag became rather pointless. |
karajorma (administrator) 2010-05-02 06:24 |
I committed all of the changes apart from the ones to missioncampaign.cpp. I don't see why you've replaced a pair of asserts with mfprint()s. Wouldn't that result in exactly the kind of silent error this patch is designed to kill? |
Genghis (developer) 2010-05-02 09:23 Last edited: 2010-05-02 09:24 |
My thinking was that if something is bad enough to trigger an assert it's fatal and the program cannot continue, whereas these situations didn't look fatal with appropriate handling. |
karajorma (administrator) 2010-05-03 00:53 |
How about a warning then? |
![]() |
|||
Date Modified | Username | Field | Change |
---|---|---|---|
2010-03-22 13:49 | Genghis | New Issue | |
2010-03-22 13:49 | Genghis | File Added: extra_error_checking.diff | |
2010-03-22 13:49 | Genghis | Status | new => assigned |
2010-03-22 13:49 | Genghis | Assigned To | => Genghis |
2010-03-22 13:49 | Genghis | Status | assigned => confirmed |
2010-03-22 13:50 | Genghis | Status | confirmed => assigned |
2010-04-02 04:37 | FUBAR-BDHR | Note Added: 0011851 | |
2010-04-04 11:26 | Genghis | Note Added: 0011852 | |
2010-04-04 12:45 | chief1983 | Note Added: 0011854 | |
2010-04-04 16:26 | FUBAR-BDHR | Note Added: 0011855 | |
2010-04-05 22:15 | Genghis | Note Added: 0011859 | |
2010-04-05 22:16 | Genghis | Note Edited: 0011859 | |
2010-04-05 22:32 | FUBAR-BDHR | Note Added: 0011860 | |
2010-04-06 09:42 | Genghis | Note Added: 0011861 | |
2010-04-06 11:00 | chief1983 | Note Added: 0011862 | |
2010-04-06 12:29 | FUBAR-BDHR | Note Added: 0011863 | |
2010-04-06 13:45 | Genghis | Note Added: 0011864 | |
2010-04-06 14:26 | FUBAR-BDHR | Note Added: 0011865 | |
2010-04-06 14:53 | chief1983 | Note Added: 0011866 | |
2010-04-06 15:08 | FUBAR-BDHR | Note Added: 0011867 | |
2010-04-06 15:12 | chief1983 | Note Added: 0011868 | |
2010-04-09 00:58 | Genghis | Note Added: 0011871 | |
2010-04-09 00:58 | Genghis | File Deleted: extra_error_checking.diff | |
2010-04-09 00:58 | Genghis | File Added: extra_error_checking.diff | |
2010-04-09 10:49 | chief1983 | Note Added: 0011873 | |
2010-05-02 01:43 | karajorma | Note Added: 0011922 | |
2010-05-02 06:24 | karajorma | Note Added: 0011925 | |
2010-05-02 09:23 | Genghis | Note Added: 0011926 | |
2010-05-02 09:24 | Genghis | Note Edited: 0011926 | |
2010-05-03 00:53 | karajorma | Note Added: 0011929 | |
2010-07-05 03:10 | karajorma | Assigned To | Genghis => karajorma |
2010-09-14 20:54 | karajorma | Status | assigned => resolved |
2010-09-14 20:54 | karajorma | Fixed in Version | => 3.6.13 |
2010-09-14 20:54 | karajorma | Resolution | open => fixed |