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

Custom comp tick #780

Draft
wants to merge 9 commits into
base: dev
Choose a base branch
from
Draft

Custom comp tick #780

wants to merge 9 commits into from

Conversation

RadHazard
Copy link
Collaborator

The initial draft for HediffComps that tick less often than every tick. In order to avoid iterating over the PMComps every tick anyway, I had to split them out into a separate PMComp field in the def.

This draft doesn't include updating any actual comps or hediffs to use the new base classes yet, only base classes themselves.

Copy link
Collaborator

@Zeracronius Zeracronius left a comment

Choose a reason for hiding this comment

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

Looks fine to me.
As mentioned multiple times in comments, there are probably quite a number of things that can be optimized further, and maybe some namings that can be improved.
However without any actual implementations, it's hard to tell properly, and I think it's worth trying out if nothing else.

I agree that we should probably look over the entire hediff tree and clean it up. I moved over most of the main systems to the "new" hediff inheritance tree, but some things remain on the old one and that old one was never removed.

Implementing this will require going over everything else anyway.


if (_hasComps && comps != null)
{
int compCount = comps.Count; // Not caching this genuinely has a noticeable performance impact for hediffs that
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am amazed that this would have a noticeable effect considering .Count is
// Read-only property describing how many elements are in the List. public int Count { get { Contract.Ensures(Contract.Result<int>() >= 0); return _size; } }

https://github.com/microsoft/referencesource/blob/master/mscorlib/system/collections/generic/list.cs#L137

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main reason for that is that it's being called several hundred thousand times a second (because it gets called every iteration of the loop, which means once per comp + 1, per hediff, 60 times a second...). I assume that most of the time spent calling it is actually just all the dynamic dispatching overhead rather than the implementation itself.

for (var index = 0; index < compCount; ++index)
_pmComps[index]?.TickHour(ref severityAdjustment);

if (hashOffsetTick % 60000 == 0) // Once an in-game day
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could technically be gated behind % 60 checks to only have 2 modulus checks per tick.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could, yeah. I figured it was more readable this way, and I haven't tested it to see if the performance matters enough yet.

// Note that PMComps.CompPostTick() is never called. This is because that method exists only for legacy support.
// That method cannot be overridden and only does exactly what we're doing here (but slightly less efficiently)

if (hashOffsetTick % 60 == 0) // Every real-life second
Copy link
Collaborator

Choose a reason for hiding this comment

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

The interesting part here to consider is that this is only a real-life second at 1x speed with no lag.
But we could potentially increase delay on higher gameplay speeds and do more interpolation to compensate if we wanted to.
If we go by "Run once every RL tick regardless of game speed and provide a count of how many ticks has past since last call."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, I prefer that it be synced to game speed instead of real-life speed. Otherwise we get the possibility of non-deterministic behavior that depends on how fast the game is runs on your machine, which is just a headache.

I can change the comment to clarify that that's only at 1x speed without lag, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants