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 bug where GraphicsDevice.Clear() always cleared stencil and depth buffers. #2335

Merged
merged 2 commits into from
Apr 11, 2014
Merged

Fix bug where GraphicsDevice.Clear() always cleared stencil and depth buffers. #2335

merged 2 commits into from
Apr 11, 2014

Conversation

jbeshir
Copy link
Contributor

@jbeshir jbeshir commented Apr 7, 2014

This was a consequence of moving platform-specific code that prior to @eeafc23378861d1401ebdba417431b4e850e8abf only ran for the single-parameter form of Clear() into a PlatformClear() method used by all forms of Clear().

This change splits it out again.

… buffers.

This was a consequence of moving platform-specific code that prior to
eeafc23 only ran for the single-parameter
form of Clear() into a PlatformClear() method used by all forms of Clear().

This change splits it out again.
@JamesLupiani
Copy link
Contributor

Good catch, thanks! @tomspilman ?

@tomspilman
Copy link
Member

Yes... good catch as this is a bug, but I think this is the wrong fix.

GraphicsDevice.Clear() should call GraphicsDevice.PlatformClear(ClearOptions, Vector4, float, int) like it does now. GraphicsDevice.Clear() should probably always set ClearOptions.Stencil and ClearOptions.DepthBuffer as I think those are the defaults.

Then in GraphicsDevice.PlatformClear(...) we should disable ClearOptions.Stencil and ClearOptions.DepthBuffer if stencil or depth is not bound to the current target.

@jbeshir - You can either update this branch with the correct fix and we can reopen this PR or you can submit an new PR with the right fix.

@tomspilman tomspilman closed this Apr 9, 2014
@jbeshir
Copy link
Contributor Author

jbeshir commented Apr 9, 2014

That sounds good. I'll update the branch with that fix in the next couple of days.

On OpenGL platforms I believe previously it has always tried to clear the stencil and depth buffers and only had a comment noting that later it needed to be fixed to do that filtering. Is it sufficient for this PR to simply retain that comment (perhaps with a wording update) at the top of OpenGL's PlatformClear?

I am not clear at present how to implement the missing behaviour for OpenGL, so if that needs to be bundled into the same fix that might need to wait for someone else to look at it if I can't figure it out quickly.

@tomspilman
Copy link
Member

I'll update the branch with that fix in the next couple of days.

Great... thanks!

Is it sufficient for this PR to simply retain that comment
(perhaps with a wording update) at the top of OpenGL's PlatformClear?

Sure... you can't fix everything all the time. :)

I am not clear at present how to implement the missing
behaviour for OpenGL,

I'm not either which is why I wrote that comment in the first place. We can continue to live without it until someone finds the need to fix it.

@jbeshir
Copy link
Contributor Author

jbeshir commented Apr 11, 2014

I've added another commit to the branch which works in the suggested way; has a single PlatformClear, provides all options to it when calling it from the single parameter Clear, and removes options (or has a comment noting that it should be) that are non-applicable inside PlatformClear.

@tomspilman tomspilman reopened this Apr 11, 2014
@tomspilman
Copy link
Member

Ok... this looks good. Thanks for the contribution... merging.

tomspilman added a commit that referenced this pull request Apr 11, 2014
Fix bug where GraphicsDevice.Clear() always cleared stencil and depth buffers.
@tomspilman tomspilman merged commit 45339ae into MonoGame:develop Apr 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants