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

Disable the warning about mapgen in reality bubble #55299

Merged

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Feb 11, 2022

Summary

None

Purpose of change

Fixes #54732.
Fixes #54062.
Fixes #53830.

The warning message regarding loading tinymaps that overlap with map was helpful debugging some issues with the tests, but it is generating a lot of warnings for players and there's really no simple fix for them. In particular, it affects update mapgen triggered by scenarios, traps, and faction camp construction.

Note that the underlying problem still exists, and if any of these forms of mapgen add vehicles, then it will probably cause issues.

Describe the solution

Disable the warning outside of test_mode for now.

Describe alternatives you've considered

The long-term solution is to retire this use of tinymap in favour of an abstract mapgen interface that can affect either the main map if loaded, or something analogous to tinymap otherwise. But that's a long-term effort.

Testing

Unit tests.

Additional context

This warning message was helpful debugging some issues with the tests,
but it is generating a lot of warnings for players and there's really no
simple fix for them, so disable the warning outside of test_mode for now.
@Ramza13
Copy link
Contributor

Ramza13 commented Feb 11, 2022

So there is another sort of solution involving shifting the current overmap. #55021 for example touches on some of the code I added around dialog effects to prevent this warning. Instead of getting rid of the warning should we instead add this more places?

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Feb 11, 2022
@NetSysFire
Copy link
Member

#53830 should also be closed by this PR

@jbytheway
Copy link
Contributor Author

So there is another sort of solution involving shifting the current overmap.

That probably would work, but it feels like a horrible hack to me. Still, if someone wants to pursue it I won't stop them. But please consolidate the pattern and don't duplicate the code so it's easy to find all the places we're doing it.

IMO it's fundamentally wrong that tinymap inherits from map. If anything it should be the other way around (or, more likely, two classes inheriting from a common base class). It's like the (now-resolved) problem of npc inheriting from player (and possibly nearly as annoying to fix). This is not only a code organization issue but also a performance issue because it makes mapgen slower by making tinymap carry all the baggage of map (although this doesn't really matter except for the one unit test that does lots and lots of mapgen).

Here's a more detailed version of the approach I envision to fix this:

  • Introduce a new reality_bubble class (name subject to bikeshedding) that inherits from map.
  • Update many places using map to use reality_bubble instead.
  • Make map an abstract base class.
  • Move all the stuff that isn't needed for tinymap out of map into reality_bubble.
  • Create a new class (maybe something like omt_for_mapgen) which can be used for mapgen (and update_mapgen). When pointed at a particular OMT this class would check whether that OMT is in the reality bubble. If it is, then it forwards all mapgen operations to reality_bubble. If it's not, then it loads a tinymap for the OMT and sends the mapgen operations there.

But all of that is much easier said than done, and I'm probably not going to pursue anything like that in the near future (certainly not before the next stable release). Your suggestion is much more pragmatic and might be good if there are actual issues that need fixing in the short term. Do any of the above update mapgen operations actually place vehicles? Maybe the scenario starts?

@Maleclypse Maleclypse added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Map / Mapgen Overmap, Mapgen, Map extras, Map display labels Feb 12, 2022
@ZhilkinSerg ZhilkinSerg merged commit 261d771 into CleverRaven:master Feb 12, 2022
@PatrikLundell
Copy link
Contributor

I believe the game spawns vehicles post map gen in a number of cases (which operations they use I don't know):

I believe I've seen someone hack their starts to include a vehicle, and I would guess starts like Helicopter Crash includes a wreck. There was also an issue recently where some golf course start frequently started inside a vehicle, but that was probably because the map contained vehicles, not the scenario as such.

If I remember correctly, the merchants outside of Hub01 had vehicles with them in 0.E-0 (the only time I've encountered them so far), but I'm not sure of that, and the Tacoma Farm spawns vehicles as part of the development of the facility.

@jbytheway jbytheway deleted the disable_tinymap_load_warning branch February 12, 2022 17:57
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 <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display
Projects
None yet
6 participants