NeoForge icon indicating copy to clipboard operation
NeoForge copied to clipboard

Unnecessary `useCombinedDepthStencilAttachment` client config value

Open PepperCode1 opened this issue 11 months ago • 4 comments

The config value can be found here.

The only place it used in NeoForge is here.

To me to is strange that the user should be able to configure this. There is no case where changing the config value can resolve a hardware incompatibility. The config also affects all framebuffers. It would seem more appropriate to be able to configure this per-framebuffer as a parameter to enableStencil.

However, even that is not necessary. A GL documentation page for glFramebufferTexture2D here says the following.

Attaching a level of a texture to GL_DEPTH_STENCIL_ATTACHMENT is equivalent to attaching that level to both the GL_DEPTH_ATTACHMENT and the GL_STENCIL_ATTACHMENT attachment points simultaneously.

Therefore, the config actually does nothing, as both code paths have the same result. In my opinion, the config value should be removed and the separate attachment approach should always be used.

PepperCode1 avatar Mar 13 '24 20:03 PepperCode1

This was added by @gigaherz in https://github.com/neoforged/NeoForge/commit/8e5138da2757c1ac71d104180bd0984fbcfaffb7

I don't see a related PR, so he'll have to comment on why it exists.

Shadows-of-Fire avatar Mar 14 '24 05:03 Shadows-of-Fire

I can't remember all the details about this, but there was some reason why the combined code path was crashing some GPUs. I'll try to find discord logs later.

gigaherz avatar Mar 14 '24 06:03 gigaherz

The conversation happened on a private channel. Link: https://discord.com/channels/313125603924639766/584953713060347914/728399383112187906

It appears that it actively errored in my PC, and separating the attachments fixed the error. I do not know what reason there was for it, only that separating the attachment fixed the error.

gigaherz avatar Mar 14 '24 07:03 gigaherz

@gigaherz @PepperCode1 So what is the stance? Should we remove the config and stuff and see if it errors at all for anyone?

TelepathicGrunt avatar Jul 25 '24 00:07 TelepathicGrunt