CGAL / cgal

The public CGAL repository, see the README below
https://github.com/CGAL/cgal#readme
Other
4.87k stars 1.38k forks source link

Incorrect rendering of polyhedrons in CGAL::draw #6099

Closed dpapavas closed 2 years ago

dpapavas commented 2 years ago

I have an issue that is similar (probably the same) to the one described in #5691. The problem seems to be caused by the --old argument passed here.

From what I could glean from the message and comments of the commit that introduced it (bace15874b12ef86f96e3f442e263283b823497c), this seems to have the intent of forcing a legacy GL context, but it's not what it actually does, as far as I can see. From here, it seems that it simply overrides the detection of the actually generated context and forces the use of shaders for an older context. The result in my case, is that a 4.3 context is still created, but older shader are used. This used to result in a bunch of shader compilation errors until recently when, after a driver upgrade or something of the kind, I simply get a white screen (which I suspect may be a rendering of a white polyhedron on a white background).

Removing the --old argument from argv and setting argc to 1 fixes the rendering for me, but since this is probably highly platform and hardware dependent, you might want to run a few tests of your own. Let me know if you'd like me to create a PR for you to try out.

Edit: forgot to add that this hack is not used for the Nef_polyhedron_3 and Surface_mesh drawing code, which worked fine for me, so, unless you have reason to believe that Nef and mesh drawing may not be working for others, this would be an indication that we could simply do away with the --old flag altogether.

gdamiand commented 2 years ago

I observed the same problem with the --old argument.

The draw function of polyhedron has the "--old" parameter and the example does not draw anything; while the draw of surface_mesh does not have it and works. Removing the "--old" parameter of draw_polyhedron solves the error.

By the way, it seems strange to have this --old parameter only for some viewer?

dpapavas commented 2 years ago

My guess was that this is a "historic artifact", meaning that it was introduced to get the viewer to work around some buggy hardware or driver at one point and didn't spread further, because it was no longer necessary. This doesn't seem to be the case, at least not entirely.

It was initially introduced and only provided as an option to the Polyhedron demo application (here). It was then hardcoded into the polyhedron viewer only in 121a23d964d47e244bd3318464c332a52193fd68. The reason for this is not entirely clear and the commit message is not very illuminating but, given that the rest of the changes seem to be about camera manipulation, I'll revert to my initial guess, that it was placed there to get the viewer to work on the particular machine.

lrineau commented 2 years ago

Maybe it was committed by error.

In our OpenGL code, we have two groups of shaders: the "main" one, and a fallback one for hardware or drivers that do not implement the level of OpenGL we need (for our transparency passes, for example). Using Qt functions, the choice of the group of shader is made automatically. But, as developers, we needed to be able to debug the fallback shaders, even on our machines. That option --old was made for that: force the use of the fallback shaders, even if Qt reports that the driver and hardward of the local machine can use the main shaders.

Probably Maxime used --old locally to debug something, and he committed that change by error.

gdamiand commented 2 years ago

@lrineau Is it ok for you if I remove the --old argument for each basic viewer? If you agree I will do a PR.

lrineau commented 2 years ago

@lrineau Is it ok for you if I removed the --old argument for each basic viewer? If you agree I will do a PR.

That seems OK.