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

Add a new render state middleware #1574

Closed
wants to merge 24 commits into from

Conversation

Argon4W
Copy link
Contributor

@Argon4W Argon4W commented Oct 1, 2024

This PR:

  • introduces a new render state middleware IBufferDefinition. This middleware is composed of various IBufferDefinitionParams and is completely isolated from GL states operations. This means it offers better compatibility with mods that modify the rendering backend, as they can control GL states based on these params, rather than using opaque RenderType.
  • provides a way to bake buffer definitions into RenderType for vanilla rendering pipline.
  • provides IMultiBufferSourceExtension and a default vanilla implementation for using buffer definitions to create VertexConsumer through buffer definitions.
  • provides a way to register buffer definitions into chunk buffer layers
  • provides getOrCreateChunkBuffer(IBufferDefinition) and a default vanilla implementation in SectionRenderingContext to create chunk buffer through buffer definitions.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@PepperCode1
Copy link
Contributor

Mods like Sodium/Embeddium and Flywheel don't use core shaders (ShaderInstance) to render blocks, so it would be extremely difficult for them to support these changes. Additionally, the whole IBufferDefinition abstraction seems unnecessarily verbose due to param types and values being arbitrary, meaning a mod that needs to convert a definition to some render state container other than RenderType can realistically only support params that are standardized. Neo itself would be the best place to standardize such things, at which point, to me, it would just make more sense to hardcode possible param types and values into separate methods in a single interface. Also, the "legacy" definition implementation makes the whole system less useful because it allows a consumer of an arbitrary definition to receive one that doesn't actually support any of the public methods. Mods can already try casting to vanilla's internal CompositeRenderType and extracting the information from there, if necessary, though admittedly this is not very convenient. Still, my main concern is with the core shaders.

@Argon4W
Copy link
Contributor Author

Argon4W commented Oct 2, 2024

Mods like Sodium/Embeddium and Flywheel don't use core shaders (ShaderInstance) to render blocks, so it would be extremely difficult for them to support these changes. Additionally, the whole IBufferDefinition abstraction seems unnecessarily verbose due to param types and values being arbitrary, meaning a mod that needs to convert a definition to some render state container other than RenderType can realistically only support params that are standardized. Neo itself would be the best place to standardize such things, at which point, to me, it would just make more sense to hardcode possible param types and values into separate methods in a single interface. Also, the "legacy" definition implementation makes the whole system less useful because it allows a consumer of an arbitrary definition to receive one that doesn't actually support any of the public methods. Mods can already try casting to vanilla's internal CompositeRenderType and extracting the information from there, if necessary, though admittedly this is not very convenient. Still, my main concern is with the core shaders.

IBufferDefinition isn’t exclusive to chunk buffer rendering, Shader params for the ShaderInstance is still needed. I am still finding a solution for the chunk buffer shader things.
Also, the params should not be hardcoded cuz mods like sodium may provide additional params (like allowFragmentDiscard), hardcoded it may lose the flexibility and compatibility. General params like BooleanParam, FloatParam, IntegerParam which covers most of the use case can be added to definitions through the ResourceLocation alias of the param type registered before the RegisterBufferDefinitionsEvent, without actually accessing the param type that provided by those mods. The DefaultBufferDefinition will discard the unknown alias for the reason like the provider mod is not loaded. The whole alias system is made for better compatibility than hardcoded params. By the way if mods like sodium uses their own shaders for chunk rendering, they can add their own chunk shader param types with an ResourceLocationParam (not implemented yet in the PR).
I will remove LegacyRenderTypeBufferDefinition soon cuz it’s only for testing the chunk buffer layers

@Argon4W
Copy link
Contributor Author

Argon4W commented Oct 3, 2024

Mods like Sodium/Embeddium and Flywheel don't use core shaders (ShaderInstance) to render blocks, so it would be extremely difficult for them to support these changes. Additionally, the whole IBufferDefinition abstraction seems unnecessarily verbose due to param types and values being arbitrary, meaning a mod that needs to convert a definition to some render state container other than RenderType can realistically only support params that are standardized. Neo itself would be the best place to standardize such things, at which point, to me, it would just make more sense to hardcode possible param types and values into separate methods in a single interface. Also, the "legacy" definition implementation makes the whole system less useful because it allows a consumer of an arbitrary definition to receive one that doesn't actually support any of the public methods. Mods can already try casting to vanilla's internal CompositeRenderType and extracting the information from there, if necessary, though admittedly this is not very convenient. Still, my main concern is with the core shaders.

Also, sodium uses identical shader source among all terrain render passes with different shader macros to control the behavior when chunk rendering. It can be done in this solution by providing params that controls the macros (like FragmentDiscardParam in this PR)

@Argon4W Argon4W force-pushed the 1.21.x-render-middleware branch from 06c1985 to 389fb9f Compare October 13, 2024 19:40
@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request rendering Related to rendering 1.21.3 Targeted at Minecraft 1.21.3 labels Nov 18, 2024
@neoforged-automation
Copy link

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

@embeddedt
Copy link
Member

The vast majority of this code is largely a duplication of the existing "state shards" vanilla already provides, and it's unclear what benefits it opens up to modders compared to introspecting the RenderTypes directly. In general changes to this system would be better structured as extensions to RenderType rather than an outright replacement for it (the latter is difficult to maintain).

Given all that, the current consensus from the team is that this PR will not be merged in its current state.

@embeddedt embeddedt closed this Dec 4, 2024
@Argon4W
Copy link
Contributor Author

Argon4W commented Dec 4, 2024

The vast majority of this code is largely a duplication of the existing "state shards" vanilla already provides, and it's unclear what benefits it opens up to modders compared to introspecting the RenderTypes directly. In general changes to this system would be better structured as extensions to RenderType rather than an outright replacement for it (the latter is difficult to maintain).

Given all that, the current consensus from the team is that this PR will not be merged in its current state.

The code is aiming to provide a middleware that holds data visible to mods that has their own rendering backend, instead of opaque RenderStateShard that modifies random gl states. Therefore these mods can offer buffers or render state holders based on these params instead of hardcoded render type. The default params (seems to be the duplication of existing state shards) is necessary for building a BufferDefinition that are equally functional as the original RenderType. It provides the ability to convert the middleware to the vanilla rendering backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.3 Targeted at Minecraft 1.21.3 enhancement New (or improvement to existing) feature or request rendering Related to rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants