Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable 'failed to visit member' JSON errors outside of tests #45635

Merged
merged 3 commits into from
Dec 18, 2020

Conversation

anothersimulacrum
Copy link
Member

@anothersimulacrum anothersimulacrum commented Nov 25, 2020

Summary

SUMMARY: Infrastructure "Enable JSON member visitation checks outside of tests"

Purpose of change

These errors are a little vague, but they cover a wide class of errors (typos, data in the wrong place, wrong type, etc) which tend to be quite hard to diagnose.
These were initially introduced in #34735, but disabled fairly soon after in #35065, due to #35035.

#35035 was caused in great part (or possibly entirely) by bugs in our JSON loading code that cause the game to report it is failing to visit members when it is not. When this was logged, the game also generated a full backtrace which it wrote to debug.log, a slow process, even slower on Windows.

As such, these errors need to be (and have been) fixed. Additionally, if somehow the same problem were still to exist, these now cause a debugmsg, making it clear at least that is causing problems.

This will cause problems for third party mods and tilesets in the short term, but in the long term, it will avoid many errors.
I'll fix all first party tilesets before I undraft this PR.

Addressing #45617 (comment), I'll do some profiling to see if it is a bottleneck there (I suspect not).

Describe the solution

This first commit is a duplicate of 7a510d6
Fix item::deserialize failed to visit object error
7c055c0

There were two sources of failed to visit object errors in item::deserialize.

The first occurred because a JsonObject was being fetched and immediately destroyed when checking if it had a member.
Shift this to fetch the object and keep it around to reuse later, and more importantly tell it to ignore unvisited members.

The second occurred when loading the basic data. Here, a second data type derived from a JsonObject is required. There were two issues here.

  1. The original data object did visit the same members as the derived one. This is fixed with the added copy_visited_members function.
  2. The derived type was still a JsonObject, so now we had two JsonObjects of the same data, with both visited different data.
    To fix this, allow omitted members on the derived type.

Fix more JSON failed to visit member errors
c8749c8

As they appear in the diff:

  1. I couldn't track down where the error was occurring here, so I just told the tileset load to ignore it. (It was loading correctly)

  2. This makes it more clear why the 'additional_tiles' member is not visited.

  3. Color loaders are loaded separately from their type member being loaded (but it is required elsewhere), so just visit it.

  4. In initializing the JsonObjectInputArchive, a JsonObject was created and destroyed without visiting members. Make that explicit, and tell it to ignore omitted members.

  5. Support for reactor and tank _plut was removed recently, but old saves may still have them. Just visit them, it'll be resolved with a new save.

  6. grab_type was only read if grab_point is valid, but it was unconditionally written. So read it, but don't always use it.

  7. See Friendly NPCs will now move out of the way #5

  8. This member was written, but not read.

  9. This member was written, but not read.

  10. external options have an info string explaining what they do. Visit this string, even though it doesn't contain anything the game uses. I chose to visit it instead of commenting it out to avoid massive amounts of lines changed (in mods and in data/core).

Enable failed to visit member outside of tests
4f7debf

Though this error is somewhat cryptic, it is hugely useful because it catches typos and incorrect placement of data. These are fairly common and hard to diagnose errors, and reporting these in a way that is accessible to anyone creating JSON for the game will be a big help.

This will cause problems for third party mods and tilesets in the short term, but in the long term, it will avoid many errors.

Testing

Loading existing saves gives me no errors. Loading all tilesets gives me no errors.

@anothersimulacrum anothersimulacrum added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Nov 25, 2020
@anothersimulacrum
Copy link
Member Author

Some profiling (focused on solely loading the game):
Before:
All:
image
Just JsonObjects-related:
image

After:
All:
image
Just JsonObjects-related:
image

Looks pretty inconsequential to me.

As they appear in the diff:

1. I couldn't track down where the error was occurring here, so I just
   told the tileset load to ignore it. (It was loading correctly)

2. This makes it more clear why the 'additional_tiles' member is not
   visited.

3. Color loaders are loaded separately from their type member being
   loaded (but it is required elsewhere), so just visit it.

4. In initializing the JsonObjectInputArchive, a JsonObject was created
   and destroyed without visiting members. Make that explicit, and tell
   it  to ignore omitted members.

5. Support for reactor and tank _plut was removed recently, but old
   saves may still have them. Just visit them, it'll be resolved with a
   new save.

6. grab_type was only read if grab_point is valid, but it was
   unconditionally written. So read it, but don't always use it.

7. See CleverRaven#5

8. This member was written, but not read.

9. This member was written, but not read.

10. external options have an info string explaining what they do. Visit
    this string, even though it doesn't contain anything the game uses.
    I chose to visit it instead of commenting it out to avoid massive
    amounts of lines changed (in mods and in data/core).
Though this error is somewhat cryptic, it is hugely useful because it
catches typos and incorrect placement of data. These are fairly common
and hard to diagnose errors, and reporting these in a way that is
accessible to anyone creating JSON for the game will be a big help.

This will cause problems for third party mods and tilesets in the short
term, but in the long term, it will avoid many errors.
These either don't have an upstream, or I couldn't figure out how to
push changes to their upstream.
@@ -108,7 +108,7 @@ void JsonObject::mark_visited( const std::string &name ) const
void JsonObject::report_unvisited() const
{
#ifndef CATA_IN_TOOL
if( test_mode && report_unvisited_members && !reported_unvisited_members &&
if( report_unvisited_members && !reported_unvisited_members &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big issue, but these two boolean variables have very similar names so it's easy to confuse them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to change it (because I'm lazy and it avoids churn), but not strongly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants