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

Don't do map processing of critters off the map #79336

Merged
merged 10 commits into from
Jan 27, 2025

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented Jan 25, 2025

Summary

None

Purpose of change

Address problems caused by loading of critters outside of the reality bubble into the creature tracker (for explosion off map handling).

Describe the solution

Don't do monster move processing of critters outside the reality bubble. Processing caused critters to teleport to the bubble map edge and spread out if the map edge position was unavailable.
Don't process critters off the map in the obstacle cache handling, and use the position on the map processed rather than just assume it's the official reality bubble map.
Load NPCs onto explosion maps.
Ensure killed monsters and NPCs both drop their loot and corpses where they are on the explosion map rather than trying to drop them on the reality bubble map (and fail to produce anything because their positions are off that map).

Describe alternatives you've considered

Edit:
game::load_npcs() got a functional change in the usage of overmap_buffer.get_npcs_near() instead of overmap.get_npcs_near_player(). Retaining the original operation if the map was the official reality bubble map would have been possible, but the reason it was replaced for that case as well is that if the PC is in the center submap of the bubble (as is usually the case), the operations yield the same result, but when the PC is off center (which happens in some rare cases, although it typically gets adjusted quickly, and those cases might not even apply when load_npcs() is called), NPCs would be loaded only on the areas covered by the map, but not the search area outside of it (due to an inbounds check), so it would cut down on the NPCs loaded (those outside the search area but inside the map) without adding any new ones.
I have not made any attempt to check whether the other usages of overmap.get_npcs_near_player() are sound, but only seen it's used in a number of places (checking it wasn't orphaned).

Testing

Using #79303 which provides the means to produce the issue (the new mortar and mortar rounds):

  • Set up "targets" in the form of breathers.

  • Walk away from the targets, sufficiently far that they're outside of the reality bubble.

  • Debug spawn mortar and ammo.

  • Set up mortar.

  • Fire off a mortar round of either type.

  • See a line of breathers appear at the edge of the reality bubble.

  • Change the code.

  • Repeat the above, firing off a shrapnel round.

  • Walk to the target area and see surface area damage north of the "targets", but no targets hit (I think: didn't think to check for injury).

  • Walk back to the mortar.

  • Fire off a crater causing round at the adjacent tile to the one hit previously.

  • Walk back to the target area and see the crater as well as some of the "targets" being removed, and some remaining in place (again, didn't think to look for injuries).

  • Do the same for zombies, but teleport away to keep them in place, and verify the corpses of the dead ones appear where they should be.

  • Do the same as the previous test but for NPCs.

Additional context

Handling NPCs was a pain, as it required bringing the map to use with a lot of operations.

While doing the work, it was noticed a lot of map operations are actually incorrect: they frequently use reality bubble references such as pos_bub and operations that use reality bubble references implicitly when their reference should be their own map object. Dealing with this will be a task for future PRs. It can be noted that this lazy coding doesn't matter as long as the operations aren't used with any maps other than the reality bubble map, which has been the case up until now.

It can also be noted that this PR probably doesn't manage to cover everything affected. There are magic effects that can trigger on death, for instance.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Missions Quests and missions Map / Mapgen Overmap, Mapgen, Map extras, Map display Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` Monsters Monsters both friendly and unfriendly. Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Melee Melee weapons, tactics, techniques, reach attack Items: Archery Bows, crossbows, arrows, bolts Items: Containers Things that hold other things Mechanics: Enchantments / Spells Enchantments and spells EOC: Effects On Condition Anything concerning Effects On Condition labels Jan 25, 2025
@github-actions github-actions bot requested a review from KorGgenT January 25, 2025 08:59
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 25, 2025
@GuardianDll
Copy link
Member

Please resolve branch conflicts

@PatrikLundell
Copy link
Contributor Author

Thanks for the heads up. I had missed it.

@PatrikLundell
Copy link
Contributor Author

Error report from tests:
Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/magic_spell_effect.cpp:1824:5: error: Avoid auto in declaration of 'here'. [cata-almost-never-auto,-warnings-as-errors]
1824 | auto &here = get_map(); // "map" is overridden by a spell_effect operation...

Is there a way to work around the override, tell the tests to not complain, or do we have to rename the offending operation?
| ^~~~~~
| map &

@GuardianDll
Copy link
Member

does class map &here = get_map(); works?

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 25, 2025
@SurFlurer
Copy link
Contributor

tell the tests to not complain

I think this will probably suppress the warning.

// NOLINTNEXTLINE(cata-almost-never-auto)
auto &here = get_map();

@akrieger
Copy link
Member

Yes you can suppress but that's a canonical example of why we have that lint. Just do map&.

@PatrikLundell
Copy link
Contributor Author

@akrieger: I don't understand your comment. A lint that tells you to change to code to something that doesn't compile isn't exactly a canonical example of something useful.

The reason I went for the ugly "auto" was precisely because "map &" doesn't work because the compiler somehow think "map" means "map()", which obviously doesn't mean anything in this context, but the lint is too primitive to pick that up.
A lint that tells you to change the code to something that doesn't work is hardly a canonical example of why it's needed (another example of a useless lint is when "tripoint_a_b(x, y, z))" is rejected with a useless claim that an unnecessary object is created, without even providing any indication of how you can perform a type conversion of this type without is resulting in a new object. The work around for that is so far to create the "redundant" object outside the operation call, which doesn't get rid of it, but causes the lint to shut up) .
However, @GuardianDll's suggestion works, and that's a good work around the underlying issue.

@akrieger
Copy link
Member

oh you mean map is another symbol in the current scope so map& gets misparsed. My mistake.

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 26, 2025
@GuardianDll GuardianDll merged commit 5de30ed into CleverRaven:master Jan 27, 2025
27 of 28 checks passed
@PatrikLundell PatrikLundell deleted the explosion branch January 27, 2025 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. EOC: Effects On Condition Anything concerning Effects On Condition Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Items: Archery Bows, crossbows, arrows, bolts Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display Mechanics: Enchantments / Spells Enchantments and spells Melee Melee weapons, tactics, techniques, reach attack Missions Quests and missions Monsters Monsters both friendly and unfriendly. NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants