MinecraftForge icon indicating copy to clipboard operation
MinecraftForge copied to clipboard

add ShaderTransformEvent to allow multi mods to transform shader resoures together

Open zomb-676 opened this issue 3 years ago • 7 comments

add ShaderTransformEvent to allow multi mods to transform shader resources together

implementation:

add event class ShaderTransformEvent which is fired before shader sources are passed to opengl holds fields below

  • private final String originShader;
  • private final Program.Type type; //identify shader
  • private final String shaderName; //identify shader
  • private final String shaderSourceName; //identify shader
  • private final GlslPreprocessor glslPreprocessor; //allow mods to shader sources’ validity
  • private String transformedShader; //allow multi mods to transform shader sources

test mod:

add a test mod called ShaderTransformEvent, default is disabled can change the text's color easily

public static void transformShader(ShaderTransformEvent event)
{
    if (ENABLED && event.getType() == Program.Type.FRAGMENT && event.getShaderName().equals("rendertype_text"))
    {
        event.transformShader((origin,transformed) ->
                transformed.replace("}","\tfragColor *= vec4(sin(gl_FragCoord.xy/1800),0.5,0.2)*3;\n}")
        );
}

test_mod

though resource packs can do this too, this way allows multi mods to transform shader sources together and in a more flexible way

performance

almost don't affect performance

diagnosis when the transformed shader is invalid

the log will print mods who subscribe to the event, so the range can be narrowed quickly

improve for mod compatibility

  • for Rubidium, flywheel, and other render relative mod, some of them create shaders/programs themselves, in this way, they can provide a more common way if they would other mod to transform their shader
  • for ImmersivePortalsModForForge, shimmer, and other mods who need transforming other mods'/vanilla's shader to achieve their targets

zomb-676 avatar Sep 16 '22 22:09 zomb-676

The proper way to do this is to build a dynamic preprocessing system within the scope of what the preprocessor can do. It shouldn't just be a simple string transformation. I will mark this PR as WIP if you would like to attempt such a system.

Since you know which shader you would like to modify, you could accomplish the same goal using a resource pack or changing which shader is used during this specific point in time.

ChampionAsh5357 avatar Sep 17 '22 13:09 ChampionAsh5357

the main purpose of this pr is to allow multi mods to transform shader resources together in a more compatible way without worrying about mixin/ASM conflicts

yes, what you said is right. A dynamic preprocessing system is more elegant and powerful. Maybe we can just parse and split shader resources into multi-components like glsl version, uniforms, input/output vertex layouts, import parts, and so on. Then let all mods transform one component one by one, then the next component. But even the most powerful system should be based on string procession. Before we can own that, I just want to make multi mods that can transform together possible

a resource pack can perfectly do this too as I mentioned at the end part of the test mod part. But this can only work fine when only one mod wants to change that. If multi mods' transform targets or installed vanilla shader packs(example) have overlap, then the transformation behavior can't be guaranteed.

But with the help of this event, we can process shader resources by code, much more powerful than resource packs. We can check if a specific mod exists and do the correspondent transformation according to your requirements

this pr's invasive is minimal, the compile error caused by the incorrectly transformed shader still exists without this event

zomb-676 avatar Sep 17 '22 14:09 zomb-676

Hello @zomb-676,

Functionality like this has been in the works for over 4 months now, including work of the core team. Merging this, in my opinion, way to broad in scope PR, would cause problems down the line with systems which are still coming.

I am respectfully asking if I could close this PR and ask you to join our discord, send me a Ping so I will notice you and then I can invite you to the working group for this if you want.

Greets,

Orion

marchermans avatar Sep 17 '22 15:09 marchermans

Thanks for your respect @OrionDevelopment

If you do think this will make problems for the coming system, feel free to close this thought I think even a thoughtful and considerate system may miss something and need a primitive/raw way in case of that. I am pleased if I can join.

zomb-676 avatar Sep 17 '22 16:09 zomb-676

@zomb-676, this pull request has conflicts, please resolve them for this PR to move forward.

autoforge[bot] avatar Dec 30 '22 19:12 autoforge[bot]

@zomb-676, this pull request has conflicts, please resolve them for this PR to move forward.

autoforge[bot] avatar Dec 30 '22 19:12 autoforge[bot]

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jul 17 '23 23:07 CLAassistant