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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix injection point for CoreShaderRegistrationCallback #3521

Open
wants to merge 1 commit into
base: 1.20.4
Choose a base branch
from

Conversation

Draylar
Copy link
Contributor

@Draylar Draylar commented Jan 12, 2024

Relevant issue: #3230

From 1.20.1(?) to 1.20.4, the injection point for the CoreShaderRegistrationCallback event has been broken: it triggers every time vanilla registers a core shader, rather than once during the shader reload cycle. In practice, this means if you rely on the event for loading shaders and use the event as modeled in the test suite, your shader is parsed/validated about 60 times per reload. An extra implication of this is that if your shader has any unused fields, it will spam console ~60 times telling you to remove it (compared to a single warning per reload, which is what happens in a vanilla environment).

I removed the slice in favor of injecting at the first add call. A potential adjustment might be injecting at the last add call (to load modded shaders after vanilla shaders), but I'm not sure how to hit that inject point without leaving the try/catch block.

Testing / Replicating

CoreShaderRegistrationCallback.EVENT.register(context -> {
// Register a custom shader taking POSITION vertices.
Identifier id = new Identifier("fabric-rendering-v1-testmod", "test");
context.register(id, VertexFormats.POSITION, program -> testShader = program);
});

Place a print statement in this callback, or register a shader that will intentionally print a warning (such as one with an unused uniform). The print/warning will log approximately 60 times, once per vanilla core shader registration. The callback should only be triggered once, which is what happens after this patch is applied.


Thank you in advance for reviews and/or change requests! 馃挆

From 1.20.1 to 1.20.4, the injection point for the CoreShaderRegistrationCallback event has been broken: it triggers every time vanilla loads a shader, rather than once during the shader reload cycle. The docs and usage case for this event make it clear the event should only trigger once during shader reloads. Any code using this event prior to the fix is registering/loading/parsing their shader about 60 times per reload in a vanilla environment. This commit fixes this bug and will only invoke the event once per shader reload.
@Chocohead
Copy link

I removed the slice in favor of injecting at the first add call. A potential adjustment might be injecting at the last add call (to load modded shaders after vanilla shaders), but I'm not sure how to hit that inject point without leaving the try/catch block.

You can do @At(value = "INVOKE_ASSIGN:LAST" to inject immediately after the final List#add

@LlamaLad7
Copy link

You cannot, selectors like that are only for use in @Slices.

@LlamaLad7
Copy link

You can however use a slice to get that selector functionality. This should also absolutely not use locals. I would suggest the following:

@WrapOperation(
		method = "loadPrograms",
		slice = @Slice(
				from = @At(value = "INVOKE:LAST", target = "Ljava/util/List;add(Ljava/lang/Object;)Z", remap = false)
		),
		at = @At(value = "INVOKE", target = "Ljava/util/List;add(Ljava/lang/Object;)Z", remap = false)
)
private boolean registerShaders(List<Pair<ShaderProgram, Consumer<ShaderProgram>>> programs, Object e, Operation<Boolean> original, ResourceFactory factory) throws IOException {
	boolean result = original.call(programs, e);
	CoreShaderRegistrationCallback.RegistrationContext context = (id, vertexFormat, loadCallback) -> {
		ShaderProgram program = new FabricShaderProgram(factory, id, vertexFormat);
		programs.add(Pair.of(program, loadCallback));
	};
	CoreShaderRegistrationCallback.EVENT.invoker().registerShaders(context);
	return result;
}

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.

None yet

3 participants