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

Added validation of mapgen weight #75770

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented Aug 17, 2024

Summary

None

Purpose of change

Fix #75741, i.e. too high mapgen weights resulting in it being ignored.

Describe the solution

Add validation of weight parameters when parsing the JSON.

Describe alternatives you've considered

Tackle validation of other JSON parsing. Scope creep...

Testing

Result of loading world created according to the bug report description:
DEBUG : Error: Json error: data/mods/desert_region/mapgen/waterbody.json:20:14: <color_white><color_cyan>min value out of bounds (0 - 20,000,000)

],
"//": "Weight should be removed if/when lake_shore and river (or their weights) are unhardcoded",

<color_light_red> "weight": 100000000000,

<color_cyan>▲▲▲
"object": {
"fill_ter": "t_mudcrack",

FUNCTION : load_game
FILE : C:\Cataclysm-DDA\src\main_menu.cpp
LINE : 1103
VERSION : 0.G-11455-g8bbef0c5d2-dirty

When the offending entry had its weight adjusted no report was produced and the save loaded normally.

Verified that nothing blows up with the 20 million limit (there might have been existing extreme weights).

Additional context

  • Note that I don't know WHY it fails. A large double number is converted to an int in the convoluted code mess, and that resulting number isn't > 0. However, preventing such numbers from being used should solve the problem.

  • There are lots of other places were weights are use. Consider that to be out of scope.

  • It's weird to parse double but then cast the values to int. Don't understand why a double is used at all.

  • There are lots of places were int64 are parsed and then cast to ints. Again, that's out of scope.

  • Decided on 20 million as the max limit to be reasonably sure you won't accidentally add yet another variant of something and then have things go weird. Note that I don't actually know how the weights are processed, but guess they're tallied up, in which case lots of large weights could push you over the limit. Anyway, 20 million should be more than enough for any real usage (trying to provide a huge number to drown out a base version is really a different issue: there's probably a need to have a way to reduce be base version's weight to 0 for mods). Again, out of scope.

  • Encountered the error report in debug.log after the one produced by this PR, but only when the error is reported, not when the game is loaded normally:
    13:48:31.401 ERROR : C:\Cataclysm-DDA\src\flexbuffer_json.cpp:347 [error_skipped_members] (json-error)
    Json error: data/json/monsters/bird.json:193:17: <color_white><color_cyan>Invalid or misplaced field name "copy-from" in JSON data

    "id": "mon_chickadee_chick",

    "type": "MONSTER",

<color_light_red> "copy-from": "mon_generic_chick_tiny",

<color_cyan>▲▲▲
"upgrades": { "age_grow": 7, "into": "mon_chickadee" }
},

Providing it here in case it might be a genuine error that somehow is suppressed normally. The base assumption is that this is a result of the error report disrupting the parsing somehow, though.

@github-actions github-actions bot added Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Aug 17, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 17, 2024
@PatrikLundell PatrikLundell marked this pull request as draft August 17, 2024 14:09
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 17, 2024
@NetSysFire
Copy link
Member

Tiny suggestion: specify what maxint (u32 vs u64) and/or simply list its value in the error. This would save the person a minute of figuring out what it means.

@PatrikLundell PatrikLundell marked this pull request as ready for review August 18, 2024 07:05
@PatrikLundell
Copy link
Contributor Author

@NetSysFire:
Encountered this with the desertmod world:
Screenshot (626)
Here pseudo terrain is generated in the middle of the land (the black tiles where the cursor is), which might be of interest to you, even though it's completely tangential to this PR itself.
Unfortunately, I didn't think to save the game, and I think it's the first time I've encountered it. Anyway, I doubt a save would really tell you much about what cause the issue.

@NetSysFire
Copy link
Member

I saw those myself, too but thanks for confirming that these are not related to the horrible hacks I applied.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 18, 2024
@dseguin dseguin merged commit 7c49b96 into CleverRaven:master Aug 18, 2024
41 of 48 checks passed
@PatrikLundell PatrikLundell deleted the weight branch August 19, 2024 08:43
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 <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
Development

Successfully merging this pull request may close these issues.

using a too high mapgen weight will silently ignore the chunk
3 participants