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

[SofaOpenglVisual] Fix ogl perf problem #1069

Merged
merged 5 commits into from Jun 6, 2019

Conversation

5 participants
@damienmarchal
Copy link
Contributor

commented May 29, 2019

This PR fix the opengl performance issue reported in ##1070

The fix is in OglModel.cpp/OglModel.h and is very simple, when the positions and normals array are containg double precision numbers they are converted into single precision one before sending the data to OpenGL. I'm sure we can optimize further the conversion loop...but I have not time for that (volunteer needed).

Because the code in OglModel was hard to read because of the #ifdef GLEW all around I first cleaned the whole SofaOpenglVisual code, by removing the #ifdef. I also updated the call to function with name *ARB because they are now part of any baseline opengl implementation since nearly a decade.


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@@ -28,19 +28,7 @@
#include <sofa/helper/system/config.h>
#include <string>

#if defined(SOFA_HAVE_GLEW)
# include <GL/glew.h>
#elif defined(__APPLE__)

This comment has been minimized.

Copy link
@damienmarchal

damienmarchal Jun 1, 2019

Author Contributor

Can someone try the code change in Windows & MacOS plz.

@damienmarchal damienmarchal changed the title Fix ogl perf problem [SofaOpenglVisual] Fix ogl perf problem Jun 3, 2019

@damienmarchal damienmarchal force-pushed the SofaDefrost:fixOglPerfProblem branch from 5dba3bb to fd29802 Jun 3, 2019

@bcarrez

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I tested it on OSX Mojave and this PR is miraculous. The caduceus goes from 35FPS to 130FPS by adding " forceFloat=1" to each of the 4 OglModel in the scene.
I approve this fix (once forceFloat is set to "on" by default)

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Thanks for the tests...actually I was more thinking to remove the forceFloat data field that was just here for testing purpose.

What is your opinion ?

@damienmarchal damienmarchal requested a review from fredroy Jun 3, 2019

@bcarrez

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I agree. remove it ! Keeping it would be pointless.

[SofaOpenglVisual] Remove the forceFloat data field
it was there just for testing purpose.
@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Done.

@guparan

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@fredroy Do you have some time to take a look?
[ci-build][with-all-tests]

@epernod

epernod approved these changes Jun 5, 2019

damienmarchal added some commits Jun 5, 2019

@fredroy

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

It is a good idea to finally remove the flag sofa_have_glew (it would be cool to remove it entirely in Sofa by the way), and I don't see anything wrong (code guidelines like prefixes etc, is not the point of this PR).
I dont know if we should flag this PR as ¨breaking¨ as some data have been removed (useVBO and isToPrint) ?

@fredroy

fredroy approved these changes Jun 6, 2019

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Hello @fredroy

Thanks for the review.
Removing glew everywhere is in PR #1071
I will add right now two deprecation message for the two data field that have been removed. [DONE]

If you agree can you pass it to ready & merge ?

@epernod epernod merged commit cd491ae into sofa-framework:master Jun 6, 2019

7 checks passed

Dashboard Builds triggered.
Details
[with-regression-tests] Triggered in latest build.
Details
[with-scene-tests] Triggered in latest build.
Details
centos_clang-5_options Build OK. FIXME: 0 unit
Details
mac_clang-3.5_options Build OK. FIXME: 1 unit
Details
ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit
Details
windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit, 1 scene, 3 regressions
Details

@guparan guparan added this to the v19.06 milestone Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.