Generate NPCs more safely in tests and fix multi-submap shifting #36837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
ccf350b
to
75afe1b
Compare
I-am-Erk
added
[C++]
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
75afe1b
to
6a13a4a
Compare
|
So now a different consistent Mingw error has replaced the one I fixed. Curious. I'm looking into it. |
BevapDin
reviewed
Jan 10, 2020
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
6a13a4a
to
962e01a
Compare
jbytheway
changed the title
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
be07a69
to
67f70ad
Compare
|
OK, that fix led to two more issues, but I believe they're all fixed now. |
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
67f70ad
to
6f96fbc
Compare
The tests have a clear_player helper. I wanted to use it with an NPC, so I've split it into clear_avatar for the avatar, and clear_character for a generic character. Currently clear_character actually takes a player& argument, because not all the things it clears are yet in Character, but that's the direction we're headed.
Previously npc_test.cpp had its own spawn_npc helper. Pull that out into player_helpers, so that it can be used in other tests.
We were seeing intermittent failures on Mingw in check_npc_behavior_tree. This turned out to be because the NPCs the test constructed in a rather ad hoc manner violated the invariants for the NPC class (in particular, its two position members were inconsistent). To avoid that, create the NPC via the new, safer spawn_npc function from player_helpers.h.
map::shift is only supposed to shift a single submap at a time. However, game::update_map could sometimes shift it more than one at a time. Add a check in map::shift for this error, and update game::update_map to not cause it.
Pull out the interesting part of the just-resolved bug and make a new unit test for it.
This allows tests to avoid calling set_pos on such, which is not safe, since it assumes the npc is a 'real' one.
jbytheway
force-pushed
the
test_npc_in_overmap
branch
from
6f96fbc
to
35d93e3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: None
Purpose of change
To stop the ongoing Travis failures for the Mingw test (see e.g. this one).
After some analysis (with the help of the new improved Windows backtraces, thanks @Qrox!), I tracked this to some oddities with the NPC position values in the
check_npc_behavior_treetest.Then that fix led to another obscure failure involving the active item cache, which I ultimately tracked down to being caused by shifting by more than one submap at a time.
Describe the solution
This turned out to be because the NPCs the test constructed in a rather ad hoc manner violated the invariants for the NPC class (in particular, its two position members were inconsistent).
To avoid that, create the NPC via the new, safer
spawn_npcfunction fromplayer_helpers.h. I created that by factoring out thespawn_npclambda that was previously innpc_test.cpp.The ranged balance tests had a similar problem. Fixed those by changing how
standard_npcworks. You now pass a position to its constructor, since its not safe to callset_poson it.To fix the submap shifting issue, add a checking in
map::shiftthat the function parameters are within the allowed range, and then updategame::update_mapto usemap::shiftcorrectly in more cases.Describe alternatives you've considered
Trying to make the way NPCs are spawned a little less confusing. Right now there are various
spawnandplacefunctions, and it's hard to understand how they all tie together.Could perhaps have changed the
check_npc_behavior_treetest to usestandard_npc, but I didn't know it existed at the time.Testing
Found a reliable way to reproduce a failure like that seen on Travis, and saw that it went away with this fix.
Add a regression test for the
map:shiftissue.Ran the rest of the unit tests too.
Tested briefly in-game to ensure I can walk and teleport between submaps correctly.