-
-
Notifications
You must be signed in to change notification settings - Fork 66
Fix image entity crash on setSampler #1972
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
base: master
Are you sure you want to change the base?
Conversation
| auto gpuTexture = _texture->getGPUTexture(); | ||
| if (gpuTexture != nullptr) { | ||
| _texture->getGPUTexture()->setSampler(sampler); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in the assumption that we try to render an image entity without knowing if the texture is loaded yet, and that is what is causing getGPUTexture() to return nullptr?
I am assuming that the texture is loaded by something else, while we are building the batch, and it is easier to just try and use it rather than only trying to render things that are finished loading?
I can see that we set setResourceTexture()'s texture to nullptr quite often in our code, so it must be intentional, right?
| auto gpuTexture = _texture->getGPUTexture(); | ||
| if (gpuTexture != nullptr) { | ||
| _texture->getGPUTexture()->setSampler(sampler); | ||
| } | ||
| // It's ok to pass nullptr to setResourceTexture. | ||
| batch->setResourceTexture(0, _texture->getGPUTexture()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch - sorry for not noticing this. I might suggest a simpler fix of just early exiting:
| auto gpuTexture = _texture->getGPUTexture(); | |
| if (gpuTexture != nullptr) { | |
| _texture->getGPUTexture()->setSampler(sampler); | |
| } | |
| // It's ok to pass nullptr to setResourceTexture. | |
| batch->setResourceTexture(0, _texture->getGPUTexture()); | |
| auto gpuTexture = _texture->getGPUTexture(); | |
| if (!gpuTexture) { | |
| return; | |
| } | |
| gpuTexture->setSampler(sampler); | |
| batch->setResourceTexture(0, gpuTexture); |
while calling setResourceTexture with nullptr is totally safe, it means we're still doing a draw call with an unset texture, which probably isn't desirable.
this is also closer to the intention of the early exit on line 142. in fact, a lot of that logic above should probably be brought into this if (pipelineType == Pipeline::SIMPLE) case, since procedural + material paths don't actually use _texture, but no one is really using those anyways...
Fixes #1970