-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add fallback rendering for other APIs #357
Conversation
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.
Did a first pass and left some minor comments. Is it possible to add a test?
Friendly ping to @ahcorde , @WilliamLewww , or @iche033 to take a look please.
I did a quick test with just gazebosim/gz-rendering#565 and there are some rendering issues: looks like we'll need to test this with ogre 2.3 from gazebosim/gz-rendering#553 |
Was gazebosim/gz-rendering#564 merged into gazebosim/gz-rendering#565? We need the copy to work correctly otherwise the download will be problematic. In fact we force RGBA8888 and without #564 it will trigger an out of bounds read due to a preexisting bug. |
oh I see, I didn't try this with gazebosim/gz-rendering#564 merged into gazebosim/gz-rendering#565. I'll do that first before testing with ogre 2.3 |
Testing with just gazebosim/gz-rendering#564 was not enough and I got the same result. I then tested this with gazebosim/gz-rendering#553 + ogre-next So this depends on gazebosim/gz-rendering#553 so we'll need to have ign-rendering7 + ogre 2.3 ready before merging this. |
f6e9f16
to
2807d86
Compare
2807d86
to
2a8954a
Compare
I thought this one was already merged and had a sudden shock when I couldn't see the code in I upgraded the PR to the latest changes; so it's rebased with latest main. |
This requires a version bump and it needs to be retargetted to |
Until we get native API to Qt implemented, fallback system can be used to transfer GPU (Ogre) -> CPU -> GPU (Qt) This is slow, but it works. Also simplify abstractions by merging IgnCameraTextureRhi into RenderThreadRhi. Fix codecheck errors Add documentation Use GraphicsAPI instead of CurrentGraphicsAPI Fix CMake project Signed-off-by: Matias N. Goldberg <[email protected]>
2a8954a
to
8a272bf
Compare
Rebased and retarget to main (well, this one was easy. I'd wish they'd all be like that) |
Codecov Report
@@ Coverage Diff @@
## main #357 +/- ##
==========================================
- Coverage 69.11% 68.40% -0.72%
==========================================
Files 44 45 +1
Lines 4919 4979 +60
==========================================
+ Hits 3400 3406 +6
- Misses 1519 1573 +54
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
{ | ||
void *texturePtr = nullptr; | ||
_camera->RenderTextureMetalId(&texturePtr); | ||
this->dataPtr->metalTexture = CFBridgingRelease(texturePtr); |
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.
homebrew build caught a compile error here:
error: no member named 'metalTexture' in 'gz::gui::plugins::RenderThreadRhiMetalPrivate'
Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
} | ||
|
||
///////////////////////////////////////////////// | ||
void* RenderThreadRhiMetal::TexturePtr() const | ||
{ | ||
return this->dataPtr->texturePtr; | ||
void *texturePtr = CFBridgingRetain(this->dataPtr->metalTexture); |
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.
homebrew caught another build error here. This fixes the problem for me:
- void *texturePtr = CFBridgingRetain(this->dataPtr->metalTexture);
+ void *texturePtr = (void *)CFBridgingRetain(this->dataPtr->metalTexture);
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.
Grrrr, thanks.
///////////////////////////////////////////////// | ||
void EngineToQtInterface::DestroyFallbackTexture() | ||
{ | ||
glDeleteTextures(1, &this->dataPtr->fallbackTexture); |
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.
windows build error here. Maybe the following helps
- glDeleteTextures(1, &this->dataPtr->fallbackTexture);
+ QOpenGLFunctions *glFuncs = this->dataPtr->glContext->functions();
+ glFuncs->glDeleteTextures(1, &this->dataPtr->fallbackTexture);
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.
Agree! That'd probably be the best solution!
Signed-off-by: Ian Chen <[email protected]>
builds fixed in 7d77cc0. |
🦟 Bug fix
No ticket has been assigned for this issue.
Order in which PRs must be merged
Summary
Until we get native API to Qt implemented, fallback system can be used
to transfer GPU (Ogre) -> CPU -> GPU (Qt)
This is important for getting APIs other than OpenGL to work with Qt.
This is slow, but it works.
Signed-off-by: Matias N. Goldberg [email protected]
Dependency
After the CI failed, I realized this PR depends on either gazebosim/gz-rendering#553 or gazebosim/gz-rendering#565 and thus cannot be merged until then.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.