Categorized attributes #5

tibetiroka · 2024-11-07T19:05:22Z

Categorized attribute format and engine implementation. See #9114 and #9025.

petervdmeer · 2025-03-08T22:54:31Z

rfcs/0004-categorized-attributes.md

+firing
+```
+
+It MAY include any or all of the following:


Why differentiate between those 2 lists? Why not list protection, resistance, damage and capacity in the same list?
Also, I'm missing the "always or passive" category, the costs that always appear just for having the outfit installed (or the costs that a ship by default has).

The "always or passive" is in the "capacity" category, which is not the greatest name... I chose it because it makes sense to have (CAPACITY, SHIELDS) or (CAPACITY, ENERGY) in the code.

Since pretty much every effect that can sensibly be applied passively has its own category, I don't think there is a real need for a separate "add this passively every frame" category, but I might have missed something.

(The two lists are differentiated in that implementing protection/resistance/damage needs a tiered system, and I wanted to keep the options open for simpler code. It's almost certainly not going to be split up, though.)

capacity and passive are 2 different things:

Capacity is how much the ship can have of something. (eg: what is the maximum amount of energy a ship can store?)

Passive is how much a ship uses of something every frame. (eg: how much energy does this ship by default consume every frame?)

(I also agree that passive is not really a great name, but I didn't come up with anything better yet.)

As an alternative name for passive; maybe continuous?

There is also a third one: How many the ship has of something currently (levels)?

petervdmeer · 2025-03-08T23:01:48Z

rfcs/0004-categorized-attributes.md

+none
+multiplier
+relative
+over time


Having specific over time keywords (like corrosion for hull damage over time) also has a bit of charm, so we might just let them stay as separate keywords in the datafiles, but in the engine we should map corrosion to "damage over time".

petervdmeer · 2025-03-08T23:08:53Z

rfcs/0004-categorized-attributes.md

+- Category (when is the change made?)
+- Effect (what is changed?)


Could we change "Category" to "Operation", and "Effect" to "Costs"?

Suggested change

- Category (when is the change made?)

- Effect (what is changed?)

- Operation (what is happening, what is the action we are doing or is being done?)

- Costs (what does the Operation cost in terms of energy and/or damage taken?)

The only objection I have to that is the effects describe the result as well as the cost. For shield generation, it would have a positive SHIELDS value, and a negative ENERGY value.

Right, so we would effectively support the following format:

"shield generation" shield -10 energy 20

And I guess we would also support the following "backwards-compatible" format:

"shield generation" 10 energy 20

For supporting the first format it would indeed be strange to call it costs, because the benefits are also listed. Effect is then a better name, but effect is also used for visual effects. We could consider calling it different like result (result of the operation), but I think effect is at least as good as result.

I guess it also makes sense to have this one the other way around regarding signs:

"shield generation" shield 10 energy -20 "shield generation" 10 energy -20

petervdmeer · 2025-03-08T23:16:38Z

rfcs/0004-categorized-attributes.md

+ship <name>
+	attributes
+		<category-keyword> [<value#>]
+			<effect-modifier-keyword> [<value#>]


I don't see why we want to have this version, if the version above also exists.
Having the keywords fully separate feels more flexible, and every redundant option that we don't support saves everyone time.

It's to allow for "ionization" instead of "energy|over time" in the data syntax. That's the main optional part there; I don't see anyone defining unique keywords for every modifier combination.

Let's just keep the beautiful keywords like "ionization" and "corrosion" and leave the "over time" part out of the keywords in the datafiles. I feel that the beautiful keywords will work better in the datafiles.

petervdmeer · 2025-03-08T23:17:06Z

rfcs/0004-categorized-attributes.md

+
+# Summary
+
+Provides a way of programmatically accessing attributes instead of requiring the unique, string-form name of the attribute.


This text (and some later texts) are not very clear on what we want to achieve. (Let's look at the other review comments first, and handle this review-comment last; the referenced Issues give a reasonable explanation on what we want.)

petervdmeer · 2025-03-08T23:32:35Z

rfcs/0004-categorized-attributes.md

+
+An implementation MUST allow efficient query of all _categorized attributes_ when given a _category_. It MUST allow for efficient transformation of all _categorized attributes_ of a given category, such as adding another set of matching attributes to it on a per-value basis.
+
+For instance, such an operation MUST be performed efficiently:


Yes, this is the in-engine benefit that we are looking for.
It is not directly 1 to 1 related to the keywords though, we can still remap some things during load and save if we want to.

petervdmeer · 2025-03-08T23:37:32Z

rfcs/0004-categorized-attributes.md

+```
+protection
+resistance
+damage


A weapon can damage an enemy, but an impacting weapon can also provide some "costs/resources" back to the firing ship. (I believe we have energy stealing weapons in-game.)
So besides damage we might also need to have a category called "steal" (or however it is called in-game).

I think that's implemented by having negative firing energy? So it could just use the generic FIRING category.

A negative firing energy happens every frame the weapon is fired, while stealing should only happen if the weapon actually hits something.

petervdmeer · 2025-03-08T23:39:58Z

rfcs/0004-categorized-attributes.md

+reverse thrusting
+steering
+active cooling
+ramscooping


solar collection could be added to this list, which provides energy.

We also seem to have energy generation by burning fuel, currently called fuel energy, but I think we should come up with a better name.

Yeah, fuel energy (and energy generation overall) is a bit of a pain in how it's set up at the moment. I think there isn't even an energy generation category, it's all in the passive/capacity category. I'll have to check the code about that.

petervdmeer · 2025-03-08T23:43:08Z

rfcs/0004-categorized-attributes.md

+ramscooping
+cloaking
+afterburning
+firing


overheat damage could also be added to this list.

I'm not sure? Overheat damage is just a flat damage source at this point, it can't have things like energy consumption applied.

Ah, I didn't know. I thought we would get something like this:

overheat hull <hull strength lost when overheating> energy <energy lost when overheating> fuel <fuel lost when overheating>

Currently, the game only supports overheat damage rate and overheat damage threshold. I like the idea of varied overheat damage, but that would be a pretty big change. Probably better left for a separate PR.

petervdmeer · 2025-03-08T23:44:41Z

rfcs/0004-categorized-attributes.md

+shield generation
+hull repair
+thrusting
+reverse thrusting


Should hyperjump and jump also be added here?

Yeah, that's a great idea.

petervdmeer · 2025-03-09T14:46:55Z

rfcs/0004-categorized-attributes.md

+
+## Performance
+
+Though the performance of the final form of this RFC should be better than our current performance, it would perform significantly worse while only the data format is implemented. Early tests suggest a doubling of the individual lookup cost of categorized attributes using the currently implemented method in the worst case.


I disagree that performance would be worse when only the new data format (keywords) are implemented; we can always use the new format at load-time and convert during loading to the old implementation. This should give exactly the same performance as we have now.

That's a good point.

petervdmeer · 2025-03-09T14:48:44Z

rfcs/0004-categorized-attributes.md

+
+We have to keep a list of all _old-form_ attribute names that the engine refers to using the new format. Though this list need not be updated or maintained, it represents an additional cost.
+
+This change will also heavily conflict with and other pull requests that create new _categorized attributes_, or change engine code using such attributes. They will all have to be moved to the _new-form_ syntax, creating a burden on the PR authors.


These are temporary costs, and the price we sometimes pay to improve something.

petervdmeer · 2025-03-09T14:49:54Z

rfcs/0004-categorized-attributes.md

+
+Is there a better way of managing compatibility than storing the _old-form_ to _new-form_ mappings?
+
+Is it worth using a Dictionary-style flap map over std::map for storing attributes? Is it really that much faster even for non-_categorized attributes_?


Why only consider maps? We know exactly how many attributes exist of this type (at load-time), so we can use fixed-size data structures for those attributes.

Also; this RFC now deals with the data-format (keywords) and also with the implementation. I feel that those two should be separate RFCs. (Or as tehhowch once remarked; let's first agree on the features and the data-format, and only then discuss implementation.)

Why only consider maps? We know exactly how many attributes exist of this type (at load-time), so we can use fixed-size data structures for those attributes.

Yes, but that number can be pretty big. I think at some point I worked it out to be around 3500 attributes?

Yes, but we wouldn't be making a single list with 3500 attributes, we would load the effects attributes into their own structures. (Per category) Those structures should support the fast adding which was mentioned in this RFC.

Suppose that I have the following categories and effects active for some ship:

shield generation shield 20 energy -30 heat 10 steering energy -20 heat 5 thrusting energy -30 heat 8

Then I expect that we use a datastructure like the following to store effects in memory (after loading):

class EffectSet { float shield; float energy; float heat; ... }

And on ship/outfit levels, I expect those attributes to be stored like:

Attributes EffectSet shieldGeneration; EffectSet steering; EffectSet thrusting; map<string, float> otherAttributes;

The otherAttributes can just be stored in a map, but we know exactly how many effects we have per category, so there we can use a class (or more efficient datastructure) to store the fixed set of EffectSet attributes per category.

The beauty of this way of storage is that the additions/calculations as listed elsewhere in this document can just be done on complete EffectSets. (A ship takes damage? Then take the damage EffectSet and substract it from the EffectSet that describes the ships current levels.)

This means that we are not storing the 3500 attributes in one long map/list, but that we store EffectSets together (under the category they belong to).

petervdmeer · 2025-03-09T14:51:29Z

rfcs/0004-categorized-attributes.md

+
+Is it worth using a Dictionary-style flap map over std::map for storing attributes? Is it really that much faster even for non-_categorized attributes_?
+
+Should the constraints of attribute values be enforced by the individual attribute entries, or the data store? Is the performance cost of enforcing them on every operation worth it? In what cases should we validate the input?


Validation can happen at load-time. So it should be okay to validate whatever we want to validate at load-time.

petervdmeer · 2025-03-09T14:52:00Z

rfcs/0004-categorized-attributes.md

+std::map<Category, std::set<Entry>> attributes;
+```
+
+An implementation MUST provide const iterator functionality over attributes. It MAY provide non-const iterator functionality.


Why do we need to iterate over attributes?

It's needed for a bunch of UI panels that display attributes, for PrintData, and (currently) for LocationFilter. (I think LocationFilter needs it to check things like "gaslining" on gas giants, so we could probably refactor that to not require an iterator.)

Conditions also required an iterator before (for saving games). Nowadays it only needs an iterator for non-automatic conditions. Maybe we could do something similar here.

petervdmeer · 2025-03-09T14:53:49Z

rfcs/0004-categorized-attributes.md

+
+An implementation MUST provide const iterator functionality over attributes. It MAY provide non-const iterator functionality.
+
+An implementation MUST provide efficient functionality matching that of Outfit::CanAdd for _categorized attributes_.


The requirement to be performant is even more critical when the Engine is active for realtime flight. (Not sure if you should add more requirements, or leave performance requirements for a more generic document.)

petervdmeer · 2025-03-09T14:55:16Z

rfcs/0004-categorized-attributes.md

+
+## Maintenance cost:
+
+We have to keep a list of all _old-form_ attribute names that the engine refers to using the new format. Though this list need not be updated or maintained, it represents an additional cost.


Listing costs and performance considerations as drawback is fine, but then I would also like to see a section with benefits, that explains how much easier it will be to work with attributes.

Co-Authored-By: Peter van der Meer <35403542+petervdmeer@users.noreply.github.com>

TheGiraffe3

Out of curiosity, why are the words describing what categorized attributes would do capitalized?
I don't see that in #1 or the already-existing RFC.

rfcs/0004-categorized-attributes.md

TheGiraffe3 · 2025-03-20T11:49:38Z

rfcs/0004-categorized-attributes.md

+
+A generator for the save file format MUST support _new-form_ attribute generation. It SHALL NOT generate _old-form_ syntax for _categorized attributes_. It MUST NOT generate duplicate entries of the _category_, _effect_, _modifier_ or combined _modifier_ + _effect_ keywords.
+
+A generator SHALL use a data node to represent a _category_ and _effect_ or _effect_ + _category_ combination. A generator SHOULD NOT include attributes with their default value.


In one place, this uses "and", and in one place "+". Is there a standard?

Yes; the "effect + category combination" is a special thing.

TheGiraffe3 · 2025-03-20T11:50:46Z

rfcs/0004-categorized-attributes.md

+force
+energy


Suggested change

force

energy

energy

force

Probably should have asked before hand: are these meant to be alphabetical?

Not really.

rfcs/0004-categorized-attributes.md

tibetiroka · 2025-03-20T20:04:24Z

Out of curiosity, why are the words describing what categorized attributes would do capitalized?

It's a pretty common format for standards; here's one of the formal definitions.

Co-authored-by: Loymdayddaud <145969603+TheGiraffe3@users.noreply.github.com>

Initial standard draft

39fe02c

tibetiroka mentioned this pull request Nov 7, 2024

feat(enhancement): Add support for a new, nested attribute format endless-sky/endless-sky#9114

Closed

3 tasks

Minor fixes

406bad8

petervdmeer reviewed Mar 8, 2025

View reviewed changes

petervdmeer reviewed Mar 9, 2025

View reviewed changes

review feedback

e829632

Co-Authored-By: Peter van der Meer <35403542+petervdmeer@users.noreply.github.com>

TheGiraffe3 reviewed Mar 20, 2025

View reviewed changes

tibetiroka and others added 3 commits March 20, 2025 21:06

Update rfcs/0004-categorized-attributes.md

8f013cb

Co-authored-by: Loymdayddaud <145969603+TheGiraffe3@users.noreply.github.com>

Update rfcs/0004-categorized-attributes.md

7fbe403

Co-authored-by: Loymdayddaud <145969603+TheGiraffe3@users.noreply.github.com>

Update rfcs/0004-categorized-attributes.md

cdf0955

Co-authored-by: Loymdayddaud <145969603+TheGiraffe3@users.noreply.github.com>

		- Category (when is the change made?)
		- Effect (what is changed?)


		# Summary

		Provides a way of programmatically accessing attributes instead of requiring the unique, string-form name of the attribute.


		An implementation MUST allow efficient query of all _categorized attributes_ when given a _category_. It MUST allow for efficient transformation of all _categorized attributes_ of a given category, such as adding another set of matching attributes to it on a per-value basis.

		For instance, such an operation MUST be performed efficiently:


		## Performance

		Though the performance of the final form of this RFC should be better than our current performance, it would perform significantly worse while only the data format is implemented. Early tests suggest a doubling of the individual lookup cost of categorized attributes using the currently implemented method in the worst case.


		We have to keep a list of all _old-form_ attribute names that the engine refers to using the new format. Though this list need not be updated or maintained, it represents an additional cost.

		This change will also heavily conflict with and other pull requests that create new _categorized attributes_, or change engine code using such attributes. They will all have to be moved to the _new-form_ syntax, creating a burden on the PR authors.


		Is there a better way of managing compatibility than storing the _old-form_ to _new-form_ mappings?

		Is it worth using a Dictionary-style flap map over std::map for storing attributes? Is it really that much faster even for non-_categorized attributes_?


		Is it worth using a Dictionary-style flap map over std::map for storing attributes? Is it really that much faster even for non-_categorized attributes_?

		Should the constraints of attribute values be enforced by the individual attribute entries, or the data store? Is the performance cost of enforcing them on every operation worth it? In what cases should we validate the input?


		An implementation MUST provide const iterator functionality over attributes. It MAY provide non-const iterator functionality.

		An implementation MUST provide efficient functionality matching that of Outfit::CanAdd for _categorized attributes_.


		## Maintenance cost:

		We have to keep a list of all _old-form_ attribute names that the engine refers to using the new format. Though this list need not be updated or maintained, it represents an additional cost.


		A generator for the save file format MUST support _new-form_ attribute generation. It SHALL NOT generate _old-form_ syntax for _categorized attributes_. It MUST NOT generate duplicate entries of the _category_, _effect_, _modifier_ or combined _modifier_ + _effect_ keywords.

		A generator SHALL use a data node to represent a _category_ and _effect_ or _effect_ + _category_ combination. A generator SHOULD NOT include attributes with their default value.

-force
-energy
+energy
+force

Categorized attributes #5

Are you sure you want to change the base?

Categorized attributes #5

Uh oh!

Conversation

tibetiroka commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petervdmeer Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petervdmeer Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petervdmeer Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petervdmeer Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibetiroka commented Nov 7, 2024 •

edited

Loading

petervdmeer Mar 15, 2025 •

edited

Loading

petervdmeer Mar 15, 2025 •

edited

Loading

petervdmeer Mar 9, 2025 •

edited

Loading

petervdmeer Mar 9, 2025 •

edited

Loading

petervdmeer Mar 15, 2025 •

edited

Loading

petervdmeer Mar 14, 2025 •

edited

Loading