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

introduce scoped registry holder using PhaseTracker #4194

Merged
merged 2 commits into from
Mar 29, 2025

Conversation

Faithcaio
Copy link
Contributor

@Faithcaio Faithcaio commented Mar 26, 2025

SpongeAPI | Sponge

Intruduces SpongeCommon#scopedHolder which attempts to find a RegistryHolder in the current cause.
SpongeCommon#vanillaRegistry now uses this.
Fallback to Server -> Game depending on availability.

During Startup SpongeLifecycle#processerverRegistries the current RegistryHolder is pushed on the stack allowing plugins during registration to access registries earlier.
Later when DedicatedServer#initServer is called the Server is pushed on the stack.

@Faithcaio Faithcaio force-pushed the feature/scopedregistryholder branch from f0ad0c9 to 799348a Compare March 27, 2025 17:07
@@ -100,8 +102,21 @@ public static RegistryAccess.Frozen vanillaRegistryAccess() {
return SpongeCommon.server().registryAccess();
}

public static SpongeRegistryHolder scopedHolder() {
var holder = PhaseTracker.getInstance().currentCause().first(SpongeRegistryHolder.class);
Copy link
Member

Choose a reason for hiding this comment

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

I still strongly feel that this is violating Cause. We need a static area to store this field, separate from an engine per my original idea. So, it is perfectly fine to put it on the PhaseTracker as an optional field. But not in the Cause. Cause is meant to be inspected, and plugin developers are told to take it as face value. Not for us to use it as a dumping ground.

Copy link
Member

Choose a reason for hiding this comment

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

Not to mention, you are creating frames where they were not needed before. If you put the field on the tracker directly, no need for these pointless frames.

Copy link
Contributor Author

@Faithcaio Faithcaio Mar 27, 2025

Choose a reason for hiding this comment

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

the frames are created 6 times on async threads

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I guess now that I realized I never needed to push the Server at #initServer I could just put a try finally block around there

@Faithcaio Faithcaio merged commit e624c20 into api-14 Mar 29, 2025
12 checks passed
@Faithcaio Faithcaio deleted the feature/scopedregistryholder branch March 29, 2025 08:11
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