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 tests by delaying get render engine calls #535

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 14, 2023

🦟 Bug fix

Targets #429

Summary

Changes:

  • moved rendering::engine calls to after the window had a chance to load the engine in tests. Otherwise the engine will be loaded in the main test thread and subsequent render calls will fail.
  • reinstated calls to unload engine

This does not fix the INTEGRATION_camera_tracking test as it could be a different issue. I haven't been able to reproduce that locally yet.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

@iche033 iche033 requested a review from jennuine as a code owner April 14, 2023 18:39
@iche033 iche033 mentioned this pull request Apr 14, 2023
8 tasks
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #535 (67d02aa) into fix_tests (f81686a) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 67d02aa differs from pull request most recent head acfcafc. Consider uploading reports for the commit acfcafc to get more accurate results

@@              Coverage Diff              @@
##           fix_tests     #535      +/-   ##
=============================================
- Coverage      69.19%   69.13%   -0.07%     
=============================================
  Files             44       44              
  Lines           4938     4938              
=============================================
- Hits            3417     3414       -3     
- Misses          1521     1524       +3     
Impacted Files Coverage Δ
src/plugins/minimal_scene/MinimalScene.hh 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@iche033
Copy link
Contributor Author

iche033 commented Apr 14, 2023

hmm tests are fixed on focal but still failing on jammy

@azeey
Copy link
Contributor

azeey commented Apr 17, 2023

Looking at https://doc.qt.io/qt-5/qthread.html, it says

It is important to remember that a QThread instance lives in the old thread that instantiated it, not in the new thread that calls run(). This means that all of QThread's queued slots and invoked methods will execute in the old thread. Thus, a developer who wishes to invoke slots in the new thread must use the worker-object approach; new slots should not be implemented directly into a subclassed QThread.

But our RenderThread class does implement a few slots even though it's a QThread. Should RenderThread be implemented using the worker-object approach?

@iche033
Copy link
Contributor Author

iche033 commented Apr 21, 2023

the implementation is based on an Qt example showing how to integrate opengl apps into QML. In that example, it also uses a QThread with slots. I can't seem to find the example any more so it could be outdated. There is this new example: https://doc.qt.io/qt-5/qtquick-scenegraph-openglunderqml-example.html. The approach is different. It maybe worth investigating the new approach later, e.g if we decide to upgrade to Qt6.

@azeey
Copy link
Contributor

azeey commented Apr 21, 2023

Okay, I took a closer look and I see that our RenderThread class does not implement run, which means the default implementation will be used, which calls QThread::exec and starts the event loop for that object. I think it's essentially doing what the worker-object approach would be doing.

@iche033 iche033 merged commit fbfd8a0 into fix_tests Jun 6, 2023
@iche033 iche033 deleted the iche033/fix_tests branch June 6, 2023 18:59
@iche033 iche033 mentioned this pull request Aug 18, 2023
8 tasks
mjcarroll added a commit that referenced this pull request Aug 22, 2023
* Break scene3d test apart into component tests

Signed-off-by: Michael Carroll <[email protected]>

* Fix include paths

Signed-off-by: Michael Carroll <[email protected]>

* Remove scene3d as it is deprecated

Signed-off-by: Michael Carroll <[email protected]>

* Remove message header

Signed-off-by: Michael Carroll <[email protected]>

* Bump everything to ogre2 by default

Signed-off-by: Michael Carroll <[email protected]>

* line order

Signed-off-by: Louise Poubel <[email protected]>

* Fix tests by delaying get render engine calls  (#535)



---------

Signed-off-by: Ian Chen <[email protected]>

* revert default engine

Signed-off-by: Ian Chen <[email protected]>

* Fix rendering tests (#561)

* debugging

Signed-off-by: Ian Chen <[email protected]>

* reset cam

Signed-off-by: Ian Chen <[email protected]>

* more testing

Signed-off-by: Ian Chen <[email protected]>

* no unload

Signed-off-by: Ian Chen <[email protected]>

* more testing with no unload

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer debugging

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer reset scene

Signed-off-by: Ian Chen <[email protected]>

* comment out unload

Signed-off-by: Ian Chen <[email protected]>

* increase timeout

Signed-off-by: Ian Chen <[email protected]>

* test timing

Signed-off-by: Ian Chen <[email protected]>

* cleanup

Signed-off-by: Ian Chen <[email protected]>

* cleanup

Signed-off-by: Ian Chen <[email protected]>

* more cleanup

Signed-off-by: Ian Chen <[email protected]>

---------

Signed-off-by: Ian Chen <[email protected]>

* Suppress warnings in metal

Signed-off-by: Michael Carroll <[email protected]>

* Incorrect args

Signed-off-by: Michael Carroll <[email protected]>

---------

Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants