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

Fix RenderState Parameters Leaking Between Renders #2130

Merged
merged 4 commits into from
Oct 28, 2023

Conversation

codex128
Copy link
Contributor

@codex128 codex128 commented Oct 25, 2023

This PR fixes shadows and other rendering issues caused by #2091 (github issue #2129), where render state parameters were leaking between render techniques because RenderState#set does not set all parameters.

Unfortunately, I was unable to get spotlight shadows to work in my test class, but TestSpotLightShadows worked fine, so I'm assuming I simply set up spotlight shadows wrong.

Any testing is greatly appreciated.

@codex128 codex128 changed the title Fix Shadows Fix RenderState Parameters Leaking Between Renders Oct 25, 2023
@riccardobl riccardobl added hacktoberfest-accepted PRs that are Hacktoberfest valid (Optional if PR is merged or approved during the month of october) bug Something that is supposed to work, but doesn't. More severe than a "defect". labels Oct 25, 2023
@Trass3r
Copy link
Contributor

Trass3r commented Oct 25, 2023

Looks like this fixed the issue 👍🏽.

@stephengold stephengold linked an issue Oct 25, 2023 that may be closed by this pull request
@stephengold stephengold added this to the Future Release milestone Oct 25, 2023
@stephengold
Copy link
Member

Unless there's further substantive discussion, I plan to integrate this PR in about 48 hours.

@stephengold
Copy link
Member

Oh hey, I just noticed that this PR includes a new file "TestShadows.java".

That file needs a copy of the JME license at the top. It also needs javadoc describing the purpose of the test and how to determine whether the test passes or fails.

@codex128
Copy link
Contributor Author

I plan on deleting that test, because it doesn't really test anything other tests don't already cover. I built it so I could focus on the shadow debug without a bunch of other code cluttering things up.

@stephengold
Copy link
Member

back on track for integration in about 45 hours.

@capdevon
Copy link
Contributor

capdevon commented Oct 26, 2023

Hi guys, what do you think about writing the copyFrom method using a static method like for java utility libraries (eg System.arraycopy() or Objects.equals(a, b) ? It might be useful to return the copied object.

//Usage:
mergedRenderState = RenderState.copy( RenderState.DEFAULT, mergedRenderState );
//or
RenderState.copy( RenderState.DEFAULT, mergedRenderState );
public static RenderState copy(RenderState from, RenderState to) {
        to.applyBlendMode = from.applyBlendMode;
        to.applyColorWrite = from.applyColorWrite;
        to.applyCullMode = from.applyCullMode;
        to.applyDepthFunc = from.applyDepthFunc;
        to.applyDepthTest = from.applyDepthTest;
        to.applyDepthWrite = from.applyDepthWrite;
        to.applyLineWidth = from.applyLineWidth;
        to.applyPolyOffset = from.applyPolyOffset;
        to.applyStencilTest = from.applyStencilTest;
        to.applyWireFrame = from.applyWireFrame;
        to.backStencilDepthFailOperation = from.backStencilDepthFailOperation;
        to.backStencilDepthPassOperation = from.backStencilDepthPassOperation;
        to.backStencilFunction = from.backStencilFunction;
        to.backStencilMask = from.backStencilMask;
        to.backStencilReference = from.backStencilReference;
        to.backStencilStencilFailOperation = from.backStencilStencilFailOperation;
        to.blendEquation = from.blendEquation;
        to.blendEquationAlpha = from.blendEquationAlpha;
        to.blendMode = from.blendMode;
        to.cachedHashCode = from.cachedHashCode;
        to.colorWrite = from.colorWrite;
        to.cullMode = from.cullMode;
        to.depthFunc = from.depthFunc;
        to.depthTest = from.depthTest;
        to.depthWrite = from.depthWrite;
        to.dfactorAlpha = from.dfactorAlpha;
        to.dfactorRGB = from.dfactorRGB;
        to.frontStencilDepthFailOperation = from.frontStencilDepthFailOperation;
        to.frontStencilDepthPassOperation = from.frontStencilDepthPassOperation;
        to.frontStencilFunction = from.frontStencilFunction;
        to.frontStencilMask = from.frontStencilMask;
        to.frontStencilReference = from.frontStencilReference;
        to.frontStencilStencilFailOperation = from.frontStencilStencilFailOperation;
        to.lineWidth = from.lineWidth;
        to.offsetEnabled = from.offsetEnabled;
        to.offsetFactor = from.offsetFactor;
        to.offsetUnits = from.offsetUnits;
        to.sfactorAlpha = from.sfactorAlpha;
        to.sfactorRGB = from.sfactorRGB;
        to.stencilTest = from.stencilTest;
        to.wireframe = from.wireframe;
        return to;
    }

@codex128
Copy link
Contributor Author

I'm not opposed to it, but copyFrom (and even set, for that matter) is only used once throughout the whole of jme, so I don't think it's important enough to disrupt the PR to integrate.

@stephengold
Copy link
Member

The proposed static copy() method would be error-prone, since it would be easy to unintentionally reverse the arguments. Let's stick with the current design.

@stephengold
Copy link
Member

Unless there's further substantive discussion, I plan to integrate this PR in about 24 hours.

@stephengold stephengold merged commit 8fb016c into jMonkeyEngine:master Oct 28, 2023
14 checks passed
@stephengold
Copy link
Member

Thank you @codex128 for your contribution to JME!

@stephengold stephengold modified the milestones: Future Release, v3.7.0 Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to work, but doesn't. More severe than a "defect". hacktoberfest-accepted PRs that are Hacktoberfest valid (Optional if PR is merged or approved during the month of october)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shadows not working due to RenderState parameter leakage
5 participants