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

Create a ConfigureMainRenderTargetEvent for enabling stenciling #1715

Open
wants to merge 11 commits into
base: 1.21.x
Choose a base branch
from

Conversation

FiniteReality
Copy link
Member

This needed a small change to mod loading: it has been separated into two separate methods instead of the one ClientModLoader.begin to enable configuring the render target.

The API I've implemented here is somewhat a placeholder, but ideally we would be able to add additional methods for things like additional color attachments, or potentially listeners for when the render target gets resized.

@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Nov 27, 2024

  • Publish PR to GitHub Packages

Last commit published: 5f6b45d712d81b66ebfcca8d29965afc59c39875.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1715' // https://github.com/neoforged/NeoForge/pull/1715
        url 'https://prmaven.neoforged.net/NeoForge/pr1715'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1715.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1715
cd NeoForge-pr1715
curl -L https://prmaven.neoforged.net/NeoForge/pr1715/net/neoforged/neoforge/21.4.66-beta-pr-1715-feature-depth-stenciling/mdk-pr1715.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@FiniteReality FiniteReality marked this pull request as ready for review November 30, 2024 18:39
@FiniteReality FiniteReality added 1.21.3 Targeted at Minecraft 1.21.3 needs porting Patch conflicts; needs porting and updating. labels Dec 3, 2024
@FiniteReality FiniteReality force-pushed the feature/depth-stenciling branch from e354ca3 to f85ba22 Compare December 6, 2024 04:37
@FiniteReality FiniteReality added 1.21.4 Targeted at Minecraft 1.21.4 and removed needs porting Patch conflicts; needs porting and updating. 1.21.3 Targeted at Minecraft 1.21.3 labels Dec 6, 2024
@neoforged-automation
Copy link

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

@FiniteReality FiniteReality force-pushed the feature/depth-stenciling branch from b6c15f2 to 5537e4c Compare December 8, 2024 15:21
@FiniteReality FiniteReality force-pushed the feature/depth-stenciling branch 2 times, most recently from b8e0333 to ef3f2f4 Compare December 9, 2024 09:37
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

I think that the overall strategy is sound, but I would remove all of the patches to make depth disable-able. That will already reduce the amount of patching by quite a bit. I would try to reduce the patch size even further. Finally, might be nice to reactivate StencilEnableTest to at least validate that enabling the stencil buffer won't cause everything to blow up.

@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request rendering Related to rendering labels Dec 10, 2024
@neoforged-automation
Copy link

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

GlStateManager._texParameter(3553, 10242, 33071);
GlStateManager._texParameter(3553, 10243, 33071);
GlStateManager._glFramebufferTexture2D(36160, 36064, 3553, this.colorTextureId, 0);
+ if (this.useDepth) {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all hooks to make useDepth disableable.

@FiniteReality FiniteReality force-pushed the feature/depth-stenciling branch from 219fe6f to f278e8d Compare January 9, 2025 13:33
+ }
+
+ private MainTarget(net.neoforged.neoforge.client.event.ConfigureMainRenderTargetEvent e) {
+ super(e.useDepth(), e.useStencil());
Copy link
Member

Choose a reason for hiding this comment

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

useDepth shouldn't be in the event

- Changed the construction of MainTarget to use a hook instead
  - NeoForge now controls all parameters cleanly, with APIs that
    can be expanded for additional users (e.g. resolution scaling on
    Retina displays)
- Removed the use_stencil_buffer hook in PostChainConfig
  - This is unsupported on MacOS through most means:
    - Mac does not support ARB_texture_stencil8 (OpenGL 4.4)
    - Mac does not support ARB_texture_stenciling (OpenGL 4.3)
    - OpenGL 4.2 is the highest Mac supports
  - The code to wire this through remains for advanced users under the
    expectation they will perform the necessary tests themselves.
- Attempted to support Mac by falling back to a combined depth/stencil texture
  - This should be compatible in most cases as stencil-only buffers currently
    only exist for custom render targets.
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

You added even more complexity to support the stencil only case. I don't think this is the way to go. Stencil-only support should likely be moved to a separate PR so that we can focus on restoring previous behavior.

I would like more maintainers to review this PR and get their opinion. Specifically from @XFactHD whenever he can.

@embeddedt
Copy link
Member

@Technici4n I'm not really seeing much code that is specific to stencil-only mode, as opposed to being needed to support stenciling at all. Could you clarify the "even more complexity" you're referring to?

In general I haven't been following this PR too closely but this functionality has been missing, to my knowledge, since roughly 1.21.2 and it would be better to merge something that works than to keep bikeshedding it indefinitely.

@Technici4n
Copy link
Member

@embeddedt See #1830 for a version that doesn't support stencil-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.4 Targeted at Minecraft 1.21.4 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.

8 participants