Minecraftforge: GlStateManager.popAttrib() does not work properly

[TheGreyGhost]

The vanilla GlStateManager.popAttrib() doesn't work properly because it doesn't dirty the cached Attributes. This is awkward when using the TESR because the vanilla rendering settings can't be saved & restored if the GlStateManager is used (i.e. direct calls to GL11 are necessary instead)

For example:
GlStateManager.enableLighting();
System.out.print(GL11.glIsEnabled(GL11.GL_LIGHTING)); --> true
GlStateManager.pushAttrib();
GlStateManager.disableLighting();
System.out.print(GL11.glIsEnabled(GL11.GL_LIGHTING)); --> false
GlStateManager.popAttrib();
System.out.print(GL11.glIsEnabled(GL11.GL_LIGHTING)); --> true
GlStateManager.disableLighting(); // ** cached value still false so doesn't call GL11.glDisable
System.out.print(GL11.glIsEnabled(GL11.GL_LIGHTING)); --> true // ERROR

[LexManos]

Congrats, you found a potential Bug related to people using GL directly.
What are you expecting me to do about it?
I see no pr, and this is direct GL code, so.. why should we care?

[TheGreyGhost]

The point of this issue is that it forces people to use GL code directly instead of the GLStateManager abstraction layer. If you are writing a TESR or EntityFX or similar to render using WorldRenderer, you are going to need to push and pop the GLState to avoid disrupting rendering of subsequent objects.

If you're receptive to a PR, I'll create one. Could you point me to some info/instructions on how to create a PR for vanilla classes (ie patches) please?

[Kubuxu]

@LexManos he has shown a bug in MC's state system. #popAttrib() does not update caches meaning OpenGL state and state MC thinks it is gets desynchronised.

It is not direct GL code as it is using (currently bugged) MC's wrapper.

[galaharon]

@TheGreyGhost edit the files in your IDE (in the 'Forge' project) and then run the genPatches gradle task.

[RainWarrior]

This can be fixed either by maintaining separate client-side stack with all the attributes, or by pulling all the attributes from GL on popAttrib. First solution is extensive, second is expensive (in terms of performance), and we can only guess if this is the intended semantics of the vanilla code, and is actually a bug.

[TheGreyGhost]

Well vanilla hardly uses it at all, just one spot for Item Rendering using models I think. I guess it's not a bug for them. It's only a concern for us modders if we want to use GlStateManager instead of direct calls to GL11.

The patch I had in mind: in each set attribute method there is a lastChangeDepth variable corresponding to the depth of the stack at which it was last changed / cached. In the set method cache check, if the lastChangeDepth is deeper than current stack depth, the cache is dirty and should be ignored.

[Victorious3]

Well, the GlStateManager doesn't handle reverting to the state it's caching. Which makes it completely useless as of now. You still need to enable/disable individually, it will keep track of the values but doesn't do anything with it. (Syncing two states now, instead of one). @TheGreyGhost That change would make sense but this is not of Forge's concern.
Also, using the GlStateManager is practically GL11 calls, scale for example is redirecting to GL11.glScalef immediately. So no, it's not bugged, it's just useless. The only upside compared to direct GL11 is that it looks at the prior state before changing it. Don't know what Mojang has planned for it though.

[Kubuxu]

@Victorious3 The point of GlStateManager is to avoid state leakage and reduce overhead of making sure everything that needs to be is in state it should. If I knew all calls can be done with no cost (because GlStateManager will do not call GL11 if there is no need for that) I would do them just to make sure there is no state leakage.

Direct GL calls aren't gone. We still have TESRs and Rendering Events. Forge should not just "not support" direct GL calls and without them many of mods features are not possible.
This is a serious bug limiting us. It should be fixed of alternative should be considered.

[Victorious3]

@Kubuxu It hasn't changed from 1.7.10. You can still use push & pop attrib as long as you do direct calls in-between. There is no significant overhead from GL11.glEnable if you only use it when you need it. On the contrary, storing the state on the GPU is always faster. The best would be to use GLSL in the first place. (I think 1.9 has plans for shaders, 1.8 is an unfinished version. The GlStateManager looks like a bridge to those shaders) Please keep in mind that GL is a state machine by itself, well capable of doing those tasks.
Also, I don't get why everybody is so concerned about messing with the state, just don't do anything stupid and you are fine... if you disable backface culling inside your TE you should _probably_ enable it again. How is the GlStateManager supposed to get rid of that? It's still a fixed function pipeline.

[TheGreyGhost]

Created a pull request.
@Victorious3 if you (say) disable lighting, should you turn it on again or not? only way to tell is to read the state before you change it.

I'm sure Mojang have introduced the abstraction layer for a good reason! Personally I have no problem with using GL11 instead of GlStatemanager as a workaround. The problem is that modders will see the GlStateManager throughout vanilla, think "hey that's great", use pushAttrib & popAttrib, and then (like me) spend hours trying to track down why there are subtle rendering glitches every now and again. With a little bit of patching we can save hundreds of hours of people tearing their hair out.

[Victorious3]

No. 1.8 is unfinished, it's working towards the use of shaders by removing a lot of the hardcoded mess. (Which is also a reason why I wouldn't screw the block rendering before it's done & saying that GL use is disapproved while vanilla is still doing it everywhere. Just my opinion though) The fixed function pipeline is deprecated since opengl 3.
"if you (say) disable lighting, should you turn it on again or not? only way to tell is to read the state before you change it."
You disabled it, you turn it on again. As simple as that. If you want FFP to be efficient you have to make a few presumptions. One of them is that lighting in GUIs is generally disabled for example. If you were to enable it for... rendering items for example, turn it off again! Checking IF you _need_ to turn it off again is pointless because that would mean that everything that renders behind you has to do the same check! It is always turned off unless somebody _forgets_ to turn it off again. Don't stop thinking and let the GlStateManager do it because... it can't. And it shouldn't.

About FFP see here: https://www.opengl.org/wiki/Fixed_Function_Pipeline

[Kubuxu]

@Victorious3 but you don't know whether it was enabled or not before.

[TheGreyGhost]

@Victorious3 I understand what you're saying, and if the vanilla code (not to mention other folks' mods) were as predictable as that, I would agree with you. Unfortunately, bitter experience tells me otherwise :)

The efficiency argument is a red herring I think.

For the record I agree static block rendering is handled much better using models. The more of the graphics code that gets pushed into the block model and json configuration files the better.

For dynamic or complex effects like the beacon, drawing lines or leashes, etc, I think we've got a long wait before we don't need a "tessellator" (with lighting, blend control, choice of primitives) etc for that. And the more abstraction they can put between me and the shader or the render list or vbo or whatever, the happier I'll be.

[Victorious3]

@Kubuxu It generally works like this: You write some code, realize that your lighting is broken (because apparently, lighting was enabled because it's a TE, but you didn't want that as you are currently drawing something with full brightness and without normals) so what are you doing? Disable it, render, enable it again. Never use a glEnable without the corresponding glDisable, and try it without first! It only starts to break once you are leaving that path. And it is predictable. Always leave the state like you got it. At the point where you _can't_ predict it anymore, somebody screwed it up. This is like wasting energy by making your rooms free of dust by under-inflation instead of using a duster.

@TheGreyGhost There will always be a Tessellator/WR. The vertices need to come from somewhere, and need to get to the graphics card somehow. Have you ever looked at what this model system is doing? It's just caching the vertex data and sending it to the Tessellator (which adds it to a ByteBuffer), based on the block state. See BlockModelRenderer.renderModelAmbientOcclusionQuads(...) Shaders are for lighting, transformation, blending & that alike. With every layer it will get slower. So please, only use models when they suit your case. 1.7.10 will always be faster because it was drawing blocks with divide & conquer (which is terrible, just terrible), so yeah, need to find the balance between performance and abstraction. I don't think Mojang is that far into it already, if you look at 1.8, it's literally _"Oh, our last version was a year ago... Uhm, let's add some content and make it running..."._

It's all the downside of FFP, it will get better once there are shaders, just wait for them.

[Kubuxu]

@Victorious3 This is why GlStatemanager was created. So modder don't have to play lottery whether something will work or no. How can I know that some state depend on for example graphics quality. Should I handle it manually or just use working GlStatemanager.

[Victorious3]

@Kubuxu The GlStateManager was created to prepare for shaders and not because of the overhead created by glEnable. (At the point you are concerned about that, you should probably stop serializing blocks into arrays of properties first).
No, that's the OpenGlHelper. Don't throw that together. Also, the GlStateManager _still_ lets your screw things up, it will just _not_ call glEnable/glDisable _if_ you are doing it twice. (Otherwise it will do some logical NOP) It just encourages you to be lazy... and stop caring about the state, which leads to the exact problem mentioned.

Here: https://twitter.com/TheMogMiner/status/515479546439950336
Do you think it's supposed to stay like that? With the GlStateManager more or less translating to GL11 1:1? They just replaced every GL11 call with GlStateManager, said _it's fine for now_ and pushed 1.8

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.