-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(enhancement): Add support for a new, nested attribute format #9114
base: master
Are you sure you want to change the base?
Conversation
Fix C++17 build without updating Catch2 nor MacOS deployment target
Suggestions by Peter
Co-authored-by: Loymdayddaud <145969603+TheGiraffe3@users.noreply.github.com>
source/attribute/Attribute.cpp
Outdated
const string Attribute::categoryNames[] = {"shield generation", "hull repair", | ||
GetEffectName(THRUST), GetEffectName(REVERSE_THRUST), GetEffectName(TURN), GetEffectName(ACTIVE_COOLING), | ||
GetEffectName(RAMSCOOP), GetEffectName(CLOAK), "afterburner thrust", "firing", "protection", | ||
"resistance", "damage", "capacity"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this is better placed in AttributeCategory, and using a map (similar to oldtoNew below) is likely more maintainable when we add new categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AttributeCategory is an enum, so if I put it there, it would be a global. I could move it to AttributeAccessor, if that would be better.
You might want to consider splitting this PR into separate parts:
I expect the data-format and the engine-support not to match 1:1 in the end; the Splitting is not required for me, and if you do decide to split, then I also don't have a preference for which part is done first. It's just that this PR as-is is quite big. |
That's pretty much what happened. Yes, the accessors used in the engine changed, but that's the extent of the refactors done here. No new attributes get engine support, only parsing support. |
We discussed the syntax for the data-files quite a bit on Discord today. Could you already prepare the PR for the Wiki to describing the data-format already as it will be used? Then we can handle the full set of data-format changes ahead of the implementation. And does it make sense to also have an "idle" section for the "energy generation" and "heat generation" attributes? |
Feature: This PR implements the feature request detailed and discussed in issue #9025
Feature Details
This PR is the first to implement the feature mentioned above. It only adds basic engine support for the data format; enabling the new attributes will be part of a separate PR. This one is big enough already.
What this PR does:
What this PR doesn't do:
Change the way attributes are queried or handled by other classesSome queries have been changed to the new formatCurrently, this PR just maps the attributes to their old string equivalents to provide the best compatibility with existing code. It also contains some mechanics that will be enabled / used by future PRs when internal support for the new attribute format is added. In the long term, I'd like to move away from flattened attribute names entirely (for the relevant attributes).
Usage Examples
Some new data stuff that could be done using the new format (with future functionality):
This PR gives us many new possibilities without introducing new attribute "keywords".
Have a look at #9025 for more ideas.
This feature is most useful for outfit definitions, but it does also affect save files slightly. Here's a comparison of an Arfecta's attributes from a save file:
Before
After
Testing Done
Automated Tests Added
300-ish lines of unit tests to cover most of the new stuff.
I couldn't test attribute saving this way, though. (#8725 would make those tests possible.)Attribute saving is now also tested.Performance Impact
I'll test it later.