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

Migrate construction to use int_id and string_id #37436

Merged
merged 3 commits into from
Jan 31, 2020

Conversation

Qrox
Copy link
Contributor

@Qrox Qrox commented Jan 27, 2020

Summary

SUMMARY: Infrastructure "Migrate construction to use int_id and string_id"

Purpose of change

The game was using raw integer to index construction objects, which is inconsistent with the rest of the code. Even more problematically, these raw integer indices were stored to saves directly. Since these indices depend on the loading order of the JSON data, this would cause saved in-progress construction to change type when loaded back after JSON files have been changed.

Describe the solution

Migrated construction to use int_id and string_id. Integer indices from old saves are still loaded as-is and converted to int_id, since I don't see any reliable way to infer the actual object an index was pointing to when it was last saved.

On the other hand, the construction code was removing blacklisted objects in the finalization code. Since this could cause problems if the list of construction objects and construction ids are used by external code before finalization, I also added a few debug messages about accessing the list of construction objects or using construction ids before finalization. Also moved finalization of constructions before finalization of crafting recipes, since the latter uses construction objects to do auto requirements calculation.

Additionally, I also fixed the code to properly hide unavailable constructions when toggled to do so, and made it to ignore skill requirements during construction activity with debug hammerspace.

Testing

Loaded a save from before this change with in-progress constructions and they were properly loaded. Then I started new constructions, saved, and loaded, and they were also properly loaded. Tried finishing these constructions without problem. Recipe autocalculation for faction camp projects were also working as before.

Tested toggling unavailable constructions, and all unavailable constructions were hidden. Started construction with debug hammerspace but zero skills, and construction proceeded without problem.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Jan 27, 2020
@AMurkin
Copy link
Contributor

AMurkin commented Jan 27, 2020

I don't see any reliable way to infer the actual object an index was pointing to when it was last saved.

Would it make sense to create separate JSON file with an array of string_ids to map indexes of current load order to string_ids? To use it to infer the string_id.

@Qrox
Copy link
Contributor Author

Qrox commented Jan 27, 2020

The actual object an index points to depend on when the save was created and what mods were loaded, including non-official ones. So that wouldn't work. Let's just call it spilt milk.

@ZhilkinSerg
Copy link
Contributor

Yeah, it would make some sense if it was a closed list of enums, but can't do anything sane in current case.

@ZhilkinSerg ZhilkinSerg merged commit 78e6e40 into CleverRaven:master Jan 31, 2020
@ZhilkinSerg
Copy link
Contributor

@Qrox: FYI: Blueprint checks started to fail after this one, because order of entries in various requirements vectors of some blueprint recipes changed.

What I don't get though is why checks did not fail in this PR.

@Qrox
Copy link
Contributor Author

Qrox commented Feb 1, 2020

This PR was earlier than the blueprint check PR, and that's probably why it didn't fail. What confuses me is that some recipes now seem to have different requirements in addition to changed component order, which shouldn't happen since this PR should at most change the order of requirements (which I failed to realize and left as a todo in the blueprint check PR). Thanks, I'll look into it.

@Qrox Qrox deleted the construction-id branch February 1, 2020 01:08
@Qrox
Copy link
Contributor Author

Qrox commented Feb 1, 2020

It seems that requirement_data::consolidate lacks symmetry: for example, if the requirement was

[ [ "a", 1 ], [ "b", 1 ] ]
[ [ "a", 2 ], [ "b", 3 ], [ "c", 4 ] ]

The consolidated requirement becomes

[ [ "a": 3 ], [ "b", 4 ] ]

However if the requirement was

[ [ "a", 2 ], [ "b", 3 ], [ "c", 4 ] ]
[ [ "a", 1 ], [ "b", 1 ] ]

It remains unchanged after consolidation.

The only way I can think of to ensure symmetry is to only merge two lists if they have the same tool/item requirement ids, but doing so would make the requirement data complicated and fail the complexity check of deduped_requirement_data (It generates up to 3024 alternatives for some blueprints!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants