Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Impl ServerTickEvent and RenderTickEvent#158

Merged
TheGlitch76 merged 8 commits intoPatchworkMC:masterfrom
GammaWhiskey:feature/tickevents
Jul 30, 2020
Merged

Impl ServerTickEvent and RenderTickEvent#158
TheGlitch76 merged 8 commits intoPatchworkMC:masterfrom
GammaWhiskey:feature/tickevents

Conversation

@GammaWhiskey
Copy link
Contributor

@GammaWhiskey GammaWhiskey commented Jul 28, 2020

Contained herein are my efforts to eliminate every TODO from the TickEvent class!
A few things of note here:

  • Fabric API's START_SERVER_TICK event, which is what I've used to implement ServerTickEvent (trying not to reinvent the wheel here), fires slightly later than the START phase of Forge's ServerTickEvent. Here's what I mean:
    protected void tick(BooleanSupplier booleanSupplier) {
        long l = Util.getMeasuringTimeNano();
        // Forge fires here
        ++this.ticks;
        // Fabric API fires here
        this.tickWorlds(booleanSupplier);
        ...

If this is problematic, I could create a custom inject to match Forge. (This issue does not exist with Fabric API's END_SERVER_TICK, which I've also used, as both Forge and Fabric API fire their respective events at the very end of the method.)

  • As Fabric API has no render tick events, I wrote mixins to cover that territory. This about sums up the results of that endeavor:
        ...
        if (!this.skipGameRender) {
            // RenderTickEvent START phase fires here with both stock Forge and the mixin
            this.profiler.swap("gameRenderer");
            this.gameRenderer.render(this.paused ? this.pausedTickDelta : this.renderTickCounter.tickDelta, l, tick);
            this.profiler.swap("toasts");
            this.toastManager.draw();
            // RenderTickEvent END phase fires here with the mixin
            this.profiler.pop();
            // RenderTickEvent END phase fires here with stock Forge
        }
        // Could potentially mixin another (!this.skipGameRender) conditional here as a workaround
        this.profiler.endTick();
        ...

Frankly, I'm not quite sure how to mixin where Forge patches it in. However, if the event firing before this.profiler.pop() is problematic, then the workaround specified above would theoretically accomplish the same thing - albeit in a way that isn't as clean. (How important is the profiler in relation to this event?)

@kitlith
Copy link
Contributor

kitlith commented Jul 28, 2020

Ehhh. I wouldn't be too torn up if the profiler results were slightly thrown off.

However, you can inject to after a method call (and any variable assignment with the return value) with INVOKE_ASSIGN. Does that help?

@GammaWhiskey
Copy link
Contributor Author

GammaWhiskey commented Jul 28, 2020

Ehhh. I wouldn't be too torn up if the profiler results were slightly thrown off.

However, you can inject to after a method call (and any variable assignment with the return value) with INVOKE_ASSIGN. Does that help?

I was under the impression that INVOKE_ASSIGN could only be used if the method returned a value, and DisableableProfiler.pop() returns void.

@kitlith
Copy link
Contributor

kitlith commented Jul 28, 2020 •
edited
Loading

https://jenkins.liteloader.com/view/Other/job/Mixin/javadoc/index.html?org/spongepowered/asm/mixin/injection/Inject.html says that it merely has special handling for invokes that return and immediately assign, not that it can only be used in that case.

Even if it couldn't be used, you could use 'shift' to shift the injection point to after the method call.

EDIT: apparently i copied the wrong link, I need to fix that.

@GammaWhiskey
Copy link
Contributor Author

GammaWhiskey commented Jul 28, 2020

Huh. That seems to contradict the Mixin wiki:

Like BeforeInvoke, this injection point searches for invoke instructions within the target instruction list and returns instructions which match the criteria. However the method targetted must return a value.

Plus, whenever I set it to use INVOKE_ASSIGN, it fails. Either way, I can just shift the injection point, so I'll do that!

@kitlith
Copy link
Contributor

kitlith commented Jul 28, 2020

Huh, I didn't see that. Fair enough!

@rikka0w0
Copy link
Contributor

rikka0w0 commented Jul 29, 2020

if ++this.ticks; is not public, then implementing Forge's ServerTickEvent with Fabric API's START_SERVER_TICK should be fine. I don't think there are any mod uses tick as a timestamp or care about its state.

@TheGlitch76
Copy link
Member

TheGlitch76 commented Jul 30, 2020

At worst it could cause an off-by-one error, but having that cause a genuine issue is very unlikely.

TheGlitch76
TheGlitch76 reviewed Jul 30, 2020
View reviewed changes
Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

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

One nitpick about the (relatively) massive ordinal and then this is good to go.

@TheGlitch76
Copy link
Member

TheGlitch76 commented Jul 30, 2020

Thank you for the in-depth PR description by the way, it was very helpful!

@GammaWhiskey
Copy link
Contributor Author

GammaWhiskey commented Jul 30, 2020

Thank you for the in-depth PR description by the way, it was very helpful!

I always aspire to be helpful. 👈😉👈

@TheGlitch76 TheGlitch76 merged commit f76e5db into PatchworkMC:master Jul 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kitlith kitlith kitlith left review comments

@TheGlitch76 TheGlitch76 TheGlitch76 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GammaWhiskey @kitlith @rikka0w0 @TheGlitch76