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

PlotCurveItem OpenGL : avoid automatic conversion from float64 to float32 #2111

Merged
merged 2 commits into from Nov 20, 2021

Conversation

pijyoi
Copy link
Contributor

@pijyoi pijyoi commented Nov 18, 2021

There is an inefficiency in the current PlotCurveItem OpenGL implementation:

  1. temporary float64 buffer created for vertices
  2. tell PyOpenGL to use it as float32 (thus triggering a float64 - > float32 conversion)

This PR changes step (1) to use a temporary float32 buffer right from the start.
This gains a few fps with 1,000,000 points: examples\PlotSpeedTest.py --opengl --frames 3 --nsamples 1000000

Alternatively, if it is considered a bug that glVertexPointerf was used, then that should be changed instead to glVertexPointer(2, gl.GL_DOUBLE, 0, pos). However, this would then be a change in behavior to use double precision, and in addition, the calls to setting up the stencil should also be changed to double precision.

if we are going to use the float32 version of glVertexPointer, then we
should just pass in a float32 array. otherwise there will be an
automatic conversion by PyOpenGL.

on the other hand, if the intention of using float64 arrays was to have
float64 precision, then the use of glVertexPointf would be a bug.
@j9ac9k
Copy link
Member

j9ac9k commented Nov 18, 2021

Nice find @pijyoi

I'm not sure what the consequences are of going to double-precision, so I'm inclined to think that we should probably maintain the single point precision given that's how it's been.

@NilsNemitz
Copy link
Contributor

I can't help but feel that single-wide floats with a resolution of only around 1E-7 are a little weak for showing arbitrary data.

On the other hand, the placement of OpenGL in the library currently seems to be "fast at all costs", so sticking with single floats is probably justified. The viewing range and axis tick calculations would still work CPU-side, using doubles, I guess. So at least this should not require any special treatment of range limitations (like in the various breakages we get when zooming out too far).

How reliable is OpenGL support for double-wide float vertices? Are there platforms that we want to support where glVertexPointer(2, gl.GL_DOUBLE, 0, pos) is not available? That would certainly settle the question.

On a somewhat modern system, is there still a performance penalty for operation with double vertices?
I guess moving the data to graphics card memory will take twice as long...

@pijyoi
Copy link
Contributor Author

pijyoi commented Nov 18, 2021

I can't help but feel that single-wide floats with a resolution of only around 1E-7 are a little weak for showing arbitrary data.

Yes, that's correct. #2020 has an example with artifacts when using OpenGL.

On the other hand, the placement of OpenGL in the library currently seems to be "fast at all costs", so sticking with single floats is probably justified.

And also the OpenGL codepath has been untouched for ages. So not changing its behavior is the easiest course of action.

How reliable is OpenGL support for double-wide float vertices? Are there platforms that we want to support where glVertexPointer(2, gl.GL_DOUBLE, 0, pos) is not available? That would certainly settle the question.

I remember reading language somewhere that an implementation is only needed to provide 24-bits of precision.

On a somewhat modern system, is there still a performance penalty for operation with double vertices?
I guess moving the data to graphics card memory will take twice as long...

If you use CUDA with consumer graphics cards, there is a 32x performance penalty to using double precision versus single precision. So I would suspect that that would be true when using OpenGL too. Either that or the driver truncates it to float32 anyway, which still makes it compliant.

@NilsNemitz
Copy link
Contributor

I'd say that is a fairly strong point for sticking to single-wide floats, then. There is a still a lot that can be done with 23 bits of mantissa.

Is there a good place to mention the single float limitation in the docs?

@j9ac9k
Copy link
Member

j9ac9k commented Nov 18, 2021

32x performance penalty?!?!?! that's brutal

When I started reading more about cross-platform openGL code, I was coming across some articles (granted, they were ancient, like most openGL content) indicating that 64 bit float support within opengl drivers was fairly inconsistent... anyway that's a lot of reasons listed above to keep things as they are.

I am 👍🏻 on @NilsNemitz 's suggestion on adding a blurb in the docs about this limitation.

@pijyoi
Copy link
Contributor Author

pijyoi commented Nov 18, 2021

See the comments in the following link regarding double precision:
https://stackoverflow.com/questions/59740148/opengl-floating-point-precision

Basically, double precision is not used for processing even if the API allows you to provide double precision values.

@j9ac9k
Copy link
Member

j9ac9k commented Nov 18, 2021

See the comments in the following link regarding double precision: https://stackoverflow.com/questions/59740148/opengl-floating-point-precision

Basically, double precision is not used for processing even if the API allows you to provide double precision values.

I was just chatting with the moderngl maintainers and they explained more or less the same thing to me

@pijyoi
Copy link
Contributor Author

pijyoi commented Nov 19, 2021

adding a blurb in the docs about this limitation.

Where would be a good place to put blurb? enableExperimental is effectively an undocumented option.

@j9ac9k
Copy link
Member

j9ac9k commented Nov 19, 2021

I was thinking in the useOpenGL option here https://github.com/pyqtgraph/pyqtgraph/blob/master/doc/source/config_options.rst?plain=1#L29

Broadly speaking, and this is way outside the scope of this PR, but we should likely add a separate section in the docs about benefits/consequences of enabling openGL for GraphicsView.

@j9ac9k
Copy link
Member

j9ac9k commented Nov 20, 2021

Thanks @pijyoi ! I'm not an rst wizard, but can we get ordering/numbering to not be in-line in the rendered document?

https://pyqtgraph--2111.org.readthedocs.build/en/2111/config_options.html

@pijyoi
Copy link
Contributor Author

pijyoi commented Nov 20, 2021

would need some help here...

@j9ac9k
Copy link
Member

j9ac9k commented Nov 20, 2021

I'll take a look I'm a bit; once I figure it out would you like me to push to your branch?

@pijyoi
Copy link
Contributor Author

pijyoi commented Nov 20, 2021

once I figure it out would you like me to push to your branch?

Yes, please go ahead.

@j9ac9k
Copy link
Member

j9ac9k commented Nov 20, 2021

welp, I got it, it's ugly as we run into a known issue with the sphinx_rtd_theme

but the end result isn't offensive at least...

image

@j9ac9k
Copy link
Member

j9ac9k commented Nov 20, 2021

@pijyoi since I requested the documentation bit, and it's added, I'm going to merge as soon as the CI comes back green. Thanks for the PR!

@j9ac9k j9ac9k merged commit 70d70e1 into pyqtgraph:master Nov 20, 2021
@pijyoi pijyoi deleted the tweak_opengl_curve branch November 28, 2021 11:14
@gedalia
Copy link

gedalia commented Sep 9, 2022

Hi I just stumbled into this PR I've seen some funky behavior when zooming in too far to a graph that's far from y = 0
So to quickly explain: The blue here is qt painter ground truth, the Red is default opengl and the green is ogl with modified data
image
The red line is really badly quantized, I think this is because the video card just can't reasonably interpolate between these points without many more values of precision to work with.
Now the green line is what happens when I remove the lower left hand origin from the data:
x_offset = math.floor(rect.x())
y_offset = math.floor(rect.y())
gl.glTranslatef(x_offset,y_offset,0)
pos[:, 0] = x - x_offset
pos[:, 1] = y - y_offset
there's still inaccuracy in the origin of the line, but there's much less quantization in the overall interpolation. I wonder if it would just make sense to offer an option to do the model matrix xform on the CPU and then send the data down post xform.
you'll notice that the red line is actually going underneath the axis that's because the the stencil triangles have the same precision problems as the data, (I wonder if they really should use the same matrix or if there's a more reasonable pixel based model matrix to use. )

@pijyoi
Copy link
Contributor Author

pijyoi commented Sep 9, 2022

Could you open a new issue on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants