ConstantBuffer: Fallback to device parameter if GraphicsDevice is null #2246

Merged
merged 0 commits into from Mar 25, 2014

Projects

None yet

4 participants

@hach-que
Contributor

Looks like this code references the GraphicsDevice property on itself even though the device is passed in as an argument. In some situations the GraphicsDevice property can be null, which causes a crash. This allows the code to fall back to the parameter if that's the case (and everything works correctly as the device that's passed in is guaranteed to be the one being locked by the caller).

@mgbot
Member
mgbot commented Feb 24, 2014

Can one of the admins verify this patch?

@mgbot
Member
mgbot commented Feb 24, 2014

Test PASSed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/585/

@tomspilman
Member

In some situations the GraphicsDevice property can be null

What cases are those? That seems like it shouldn't happen.

@hach-que
Contributor

We had changed when an effect was being applied and this bug started appearing, even though the effect was being successfully used before. The crash occurs when referencing constant buffer 0 and when it's applying the graphics state before actually using the specified vertex / index buffers passed to DrawIndexedPrimitives (so it was crashing on the previous buffers that were still referenced, not the new ones being passed in).

@tomspilman
Member

so it was crashing on the previous buffers that were
still referenced, not the new ones being passed in

Interesting... so the previous buffers were disposed or something?

I've never investigated what XNA does if you dispose a resource that is still attached to the GraphicsDevice state. I would suspect they try to clear it off the device under the hood, but we've never done anything like that.

@hach-que
Contributor

It's possible that they were disposed; we didn't really track down how the GraphicsDevice property was being set to null, we just noticed that the device was being passed into the function and then not used at all, figuring that the correct course of action was to fall back to the device if GraphicsDevice was not set.

@tomspilman
Member

we just noticed that the device was being passed
into the function and then not used at all

Right... that does make things confusing as to which one is correct.

All resources are GraphicsDevice specific... you can't use them from another GraphicsDevice (although I have never tried to create more than one).

So that makes me think the right approach is to assert that the graphics devices matches and fix the source of the bug.... which might very well be that disposed resources are not cleared from the graphics state.

The real fix could be in ConstantBufferColection.SetConstantBuffers():

                var buffer = _buffers[i];
                if (buffer != null && !buffer.IsDisposed)
                {
#if DIRECTX
                    buffer.Apply(device, _stage, i);
#elif OPENGL || PSM
                    buffer.Apply(device, shaderProgram);
#endif
                }

Which sort of matches what we do for textures...

https://github.com/mono/MonoGame/blob/develop/MonoGame.Framework/Graphics/TextureCollection.cs#L147

But you could argue the right behavior would be to null out the entry in the collection as well.

@KonajuGames
Contributor

Why would it be passing its own GraphicsDevice to itself. The only way it would be called when GraphicsDevice is null is after it has been disposed.

@tomspilman
Member

Yea... we need to fix the cause not the symptom. The symptom here is that this.GraphicsDevice is null. The cause is most likely that we're applying IsDisposed constant buffers when those should be ignored.

@KonajuGames
Contributor

Should the GraphicsDevice parameter be removed from that method?​

@tomspilman
Member

I think it can be. I think it got there because the buffer at one time wasn't a GraphicsResource.

@mgbot
Member
mgbot commented Mar 25, 2014

Test FAILed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/643/

@tomspilman tomspilman merged commit 2937802 into MonoGame:develop Mar 25, 2014

1 check failed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment