Extract StoresResources from Harvester #20761
Conversation
PunkPun
force-pushed
the
resource-preparation
branch
from
2c83dfd
to
feb6bc1
Compare
OpenRA.Mods.Common/Traits/Render/WithStoresResourcesPipsDecoration.cs
Outdated
Show resolved
Hide resolved
PunkPun
force-pushed
the
resource-preparation
branch
2 times, most recently
from
9846d0d
to
cdee855
Compare
|
|
||
| /// <summary>Stored resources.</summary> | ||
| /// <remarks>Dictionary key refers to resourceType, value refers to resource amount.</remarks> | ||
| IReadOnlyDictionary<string, int> Contents { get; } |
There was a problem hiding this comment.
I understand you want to keep resource type a string.
On the other hand I assume this is a hot path. Seems somewhat wastefull to do constant string hash calculations and hash map lookups to get and set an integer value.
HarvesterInfo.Resources is readonly. You could assign integer indexes. So that you can get rid of of both the string and map and use an array of length Resources.
There was a problem hiding this comment.
I don't think the extra complexity is worth it. (At least unless we have some numbers on that this makes a significant impact.)
There was a problem hiding this comment.
It's not a cold path, but also it's not very hot, so such optimisations shouldn't have very noticeable impact. It may make sense to do an optimisation pass after the whole refactor is finished, but atm it would just make the code much harder to read.
There was a problem hiding this comment.
8 haversters times 8 players times any resource update.
| } | ||
| } | ||
|
|
||
| public int ContentsSum { get; private set; } = 0; |
There was a problem hiding this comment.
The line above. Is it really worth while to sync the value of each harvester contineously. And you do this by looping over the contents. I guess there are only a few types of content.
PunkPun
force-pushed
the
resource-preparation
branch
from
cdee855
to
cc1a2aa
Compare
| string[] ResourceTypes { get; } | ||
| } | ||
|
|
||
| public interface IStoresResources |
There was a problem hiding this comment.
This should also be explicitly implemented.
There was a problem hiding this comment.
Why? It would just force nasty code in other implementations
There was a problem hiding this comment.
All our interfaces should be explicit. Why would this force nasty code?
There was a problem hiding this comment.
because if the interface is explicit, you can't reference members within the class
PunkPun
force-pushed
the
resource-preparation
branch
from
cc1a2aa
to
52818a3
Compare
PunkPun
force-pushed
the
resource-preparation
branch
from
52818a3
to
2fdd16a
Compare
PunkPun
force-pushed
the
resource-preparation
branch
3 times, most recently
from
9a13333
to
b15a231
Compare
PunkPun
force-pushed
the
resource-preparation
branch
from
b15a231
to
200b3fb
Compare
PunkPun
force-pushed
the
resource-preparation
branch
from
200b3fb
to
016ad6b
Compare
PunkPun
force-pushed
the
resource-preparation
branch
from
016ad6b
to
858fa51
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This is the first PR of the resource refactor.
This was an easy first step that could be done before docking refactor or the massive systematic changes to player resources.
With this PR harvesters will be able to have multiple storage containers, although the pips will be shown for only the one (consistent with most of our UI traits). We may want to create a way of tying those traits together somehow in the future.
We may also want to consider adding per-resource conditions so enable features such as TS harvester exploding when containing blue tib. I can add this feature in this PR if requested.