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

Extend requirement groups #56323

Merged
merged 6 commits into from
Mar 27, 2022

Conversation

dseguin
Copy link
Member

@dseguin dseguin commented Mar 24, 2022

Summary

None

Purpose of change

LyleSY ran into a limitation of requirements in #56319:

Requirement groups can only be overwritten, meaning that two mods modifying the same requirement id will overwrite each other.

Describe the solution

Add an optional field to requirements to extend the list:

{
  "id": "bone_sturdy",
  "type": "requirement",
  "extend": { "components": [ [ [ "frost_bone_human", 1 ], [ "alien_bone", 1 ] ] ] }
}

Using "extend" causes the currently loaded requirement to integrate the given list.

Describe alternatives you've considered

This could be implemented as a set of requirements per id (one for each src), but that would require a significant overhaul of the rest of the codebase. This is just the simplest option.

I also tried making requirements extend by default, but inline requirement id's (generated at load time) tend to cause problems. There's no clean way to distinguish a generated id from a manually created id (other than the prefix on generated id's).

Testing

Added some test cases to check that extended requirements are properly added:

./tests/cata_test -d yes --rng-seed time "requirement_extension"

Manual testing: Loading a recipe that extends eggs_bird for DinoMod and Magiclysm.

requirement_extend

Additional context

TODO:

  • Implement "extending"
  • Update requirements in mods
  • Use extend syntax (thanks Maddremor)
  • Update documentation
  • Testing

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Mods: Aftershock Anything to do with the Aftershock mod Mods: Dinomod Anything to do with the Dinoclysm mod (DinoMod) Mods: Magiclysm Anything to do with the Magiclysm mod astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Mar 24, 2022
@Maddremor
Copy link
Contributor

I'm sure there is a good reason for this, but why not use the copy-from and extend syntax?

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 24, 2022
@dseguin
Copy link
Member Author

dseguin commented Mar 24, 2022

I'm sure there is a good reason for this, but why not use the copy-from and extend syntax?

I guess I didn't think of it :P

I'm pretty sure those are provided by generic_factory, which won't work by itself in this case. But I can make it look like the normal extend syntax, which should work like you'd expect.

@dseguin dseguin force-pushed the extend_requirement_groups branch from 62b7988 to bdd3820 Compare March 25, 2022 01:10
@github-actions github-actions bot added the <Documentation> Design documents, internal info, guides and help. label Mar 25, 2022
@dseguin dseguin force-pushed the extend_requirement_groups branch from bdd3820 to 61bf7d3 Compare March 25, 2022 02:57
@dseguin dseguin marked this pull request as ready for review March 25, 2022 02:59
@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. labels Mar 25, 2022
Copy link
Member

@Maleclypse Maleclypse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for the aftershock changes. Thank you this is really cool!

Copy link
Contributor

@LyleSY LyleSY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it thanks!

@dseguin dseguin force-pushed the extend_requirement_groups branch from 61bf7d3 to a0ae8cc Compare March 25, 2022 15:14
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 25, 2022
@dseguin dseguin force-pushed the extend_requirement_groups branch from a0ae8cc to 84321c9 Compare March 25, 2022 15:56
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 25, 2022
@LyleSY
Copy link
Contributor

LyleSY commented Mar 27, 2022

Failed monster attack test doesn't look related

@kevingranade kevingranade merged commit 2dd4029 into CleverRaven:master Mar 27, 2022
@dseguin dseguin deleted the extend_requirement_groups branch March 27, 2022 19:54
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. Code: Tooling Tooling that is not part of the main game but is part of the repo. <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mods: Aftershock Anything to do with the Aftershock mod Mods: Dinomod Anything to do with the Dinoclysm mod (DinoMod) Mods: Magiclysm Anything to do with the Magiclysm mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants