Fixes Ogl when resizing and dragging shapes

[hasii2011]

[ Many of the computations when resizing or moving OGL shapes result in float values. The result is that the wx.DC primitive calls fail because they expect integer values. This PR changes the computations results to become integer values, thus method calls on the wx device contexts do not fail

Most of the changes involve

• using the integer divide operator '//' instead of the float operator '/'
• In some instances I massage the results via the 'round' operator
• Additonally, I change the use of wx.RealPoint to just wx.Point
• Finally, I changed some of the float constants to integer constants ]

Fixes #2739

[hasii2011]

Want to get these set of changes into a daily build. If I need more patches I can work against it. I verified these changes by modifying the virtual environment I created.

[echoix]

Can you either edit the title or explain what that markup in the commit message and the PR title means?

[hasii2011]

I hope the above is clearer. Please advise

[echoix]

Couple things for the reviewers to watch for:

• The change from wx.RealPoint to wx.Point seems a bit naive for the type of fix here, it changes a big part of the meaning.
• Same thing with the overall changes in the file. Is it expected to do all intermediate calculations as integers, not just at the output? How does wxPython handle fractional scaling? The handling seems different between platforms for wxWidgets at least: https://docs.wxwidgets.org/3.2/overview_high_dpi.html
• Not fixing the line 3447 as mentioned in the thread
• Replacing a float() call with round(). Appart from changing the meaning greatly, the use of round() in the context of a graphics API surprises me. Graphics APIs usually hate off-by-one-pixel bugs, so a closer look needs to be taken to make sure the mathematical premises (assumptions) are still true after the change.

[hasii2011]

Hmm

[RobinD42]

This pull request has been mentioned on Discuss wxPython. There might be relevant details there:

https://discuss.wxpython.org/t/wx-lib-ogl-errors/40202/17

[hasii2011]

First of all I wanted to thank all of you for your patience as I migrated a community wiki discussion to a wxPython issue. Additionally, thanks for the suggestion that I open a PR and the instructions on how to do this from the discussion.

The PR not only fixes errors in my new module but also errors in the wxPython demo when moving and resizing shapes and/or lines.

The new library I am developing is based on wx.lib.ogl. It is a replacement for the miniogl module here. Currently, I am working in a virtual environment where I have patched in the contents of the PR. I am at a crux. I cannot tell if this PR is going to move forward or not.

At this point I have multiple options

1. Abandon wx.lib.ogl and proceed with a refactor of the home-grown miniogl

2. Fork the wx.lib.ogl module from wxPython

3. Abandon wxPython and look at another toolkit

 

1. I really, really like wxPython and use it in several projects PytGitIssue2Todoist and PyFabricate and Pyut. So, option 3 is very unappealing.

2. I did not want to maintain a minogl, so option 1 leaves a bad taste in my mouth.

3. I like the software structure, API, and granularity of the wx.lib.ogl module, so perhaps I might move on option 2

At this point I am asking the maintainers to either reject or accept the PR so that I can move forward

Again, thanks all of you for your help

Regards,

Humberto A. Sanchez II

[RobinD42]

This pull request has been mentioned on Discuss wxPython. There might be relevant details there:

https://discuss.wxpython.org/t/wx-lib-ogl-errors/40202/18

[hasii2011]

Currently, I am manually patching the various projects that carry these fixes

[hasii2011]

Me again.

Not sure how close the next release is our at least a dev build. At this point is there anything I can do to get this merged in. Maybe contribute to a Patreon account or something.

I have tested this on Mac OS X and on Linux Ubuntu 24.04. I see not ill effects or one off errors. Scrolling of lines and shapes is smooth.

[swt2c]

You haven't responded/addressed any of the comments made by @echoix which I agree with.

[hasii2011]

@echoix @swt2c Ok let's see if I can do this

Currently the drawing APIs (DC – device context) only accept integer values. In fact, if I look back enough, they were always integer numbers but Python was silently truncating them to integer.

With the introduction of the “//” operator Python stopped doing this automatically and the numbers were left as float. I noticed this in the “private” mini ogl library that I had been maintaining and did similar fixes there. (https://github.com/hasii2011/ogl)

The change from wx.RealPoint to wx.Point seems a bit naive for the type of fix here, it changes a big part of the meaning.

The changes from RealPoint to just Point is necessary because they are being stashed into a data structure that later on is getting passed to a DC function that expects integers. From my looking at the code it looks like the RealPoint class is a shim on top of some C++ code that always coerces the number to floats. The DC calls need integers

Same thing with the overall changes in the file. Is it expected to do all intermediate calculations as integers, not just at the output? How does wxPython handle fractional scaling? The handling seems different between platforms for wxWidgets at least: https://docs.wxwidgets.org/3.2/overview_high_dpi.html

My intention was to fix the code at the closest point to failure. W/O disturbing other code.

Not fixing the line 3447 as mentioned in the thread

I think your are talking about this line

dc.DrawEllipse(int(self._xpos - self.GetWidth() / 2.0), int(self._ypos - self.GetHeight() / 2.0), self.GetWidth(), self.GetHeight()) Replacing a float() call with round(). Appart from changing the meaning greatly, the use of round() in the context of a graphics API surprises me. Graphics APIs usually hate off-by-one-pixel bugs, so a closer look needs to be taken to make sure the mathematical premises (assumptions) are still true after the change.

Not exactly sure what you are asking me to do here. I have executed this code via some unit tests for my code that depends on this on OSX and Linux. Also have executed the visual components on OSX and Linux and have not seen any 1 off pixels errors

[swt2c]

In general, the way we've addressed these types of issues (where wxPython APIs now throw an Exception when a float is passed) previously is to simply replace the calls (at the point of the call) with an int() cast. This is sort of the minimally required, lowest risk change that replicates the behavior before Python 3.10.

For example, if you had this piece of code:

x = 1.0
y = 1.0
rp = wx.Point(x, y)

The recommended solution would be:

x = 1.0
y = 1.0
rp = wx.Point(int(x), int(y))

This is the lowest risk approach that doesn't change any of the existing calculations.

[hasii2011]

So perhaps I should close this PR and start all over again ?

[swt2c]

So perhaps I should close this PR and start all over again ?

Sure, if that's the easiest approach for you.

[echoix]

Or if we squash merge for this PR, continue and we keep the discussion and review comments at the same place

[hasii2011]

Ok, I am going to abandon this PR. I was trying to get out of the business of maintaining an OGL layer. The problems I was trying to fix are demonstrable when one tries to resize the ellipse in the demo and gets worse when one tries to drag it. I will leave it up to the maintainers to prioritize and fix.

[swt2c]

@hasii2011 can you check if the fix in #2779 is enough to resolve all the issues you're seeing?

[hasii2011]

Just checked it. I kind of expected it to fix my Ellipse reszing and dragging problems. Did not fix anything

[swt2c]

Just checked it. I kind of expected it to fix my Ellipse reszing and dragging problems. Did not fix anything

Are you sure that you applied it correctly? For me, I'm able to resize and drag stuff around in the OGL demo.

[hasii2011]

I stand corrected the demo stuff does indeed work. My code has some custom event handlers to handle resize and movement. Those are still failing with the float errors. Attempting to make a small demo program to illustrate this, but it keeps getting a SIGSEGV when I try to drag the shape

[swt2c]

I stand corrected the demo stuff does indeed work. My code has some custom event handlers to handle resize and movement. Those are still failing with the float errors. Attempting to make a small demo program to illustrate this, but it keeps getting a SIGSEGV when I try to drag the shape

If you can paste the traceback(s) of the float errors you see with your application, I can probably fix them too.

[hasii2011]

Here is one

Traceback (most recent call last):
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/frames/UmlFrame.py", line 95, in OnMouseEvent
    super().OnMouseEvent(mouseEvent)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/frames/DiagramFrame.py", line 214, in OnMouseEvent
    super().OnMouseEvent(evt=mouseEvent)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/canvas.py", line 173, in OnMouseEvent
    self._draggedShape.GetEventHandler().OnBeginDragLeft(x, y, keys, self._draggedAttachment)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 233, in OnBeginDragLeft
    self._previousHandler.OnBeginDragLeft(x, y, keys, attachment)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 1324, in OnBeginDragLeft
    self.GetEventHandler().OnDrawOutline(dc, xx, yy, w, h)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/UmlBaseEventHandler.py", line 65, in OnDrawOutline
    shape.Move(dc=dc, x=x, y=y, display=True)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 1418, in Move
    self.Draw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 1436, in Draw
    self.GetEventHandler().OnDraw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 154, in OnDraw
    self._previousHandler.OnDraw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/shapes/UmlClass.py", line 150, in OnDraw
    self._startClipping(dc=dc, leftX=x, leftY=y)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/shapes/UmlClass.py", line 509, in _startClipping
    dc.SetClippingRegion(x, y, w, h)
TypeError: DC.SetClippingRegion(): arguments did not match any overloaded call:
  overload 1: argument 1 has unexpected type 'float'
  overload 2: argument 1 has unexpected type 'float'
  overload 3: argument 1 has unexpected type 'float'
Traceback (most recent call last):
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/frames/DiagramFrame.py", line 189, in OnPaint
    self.Redraw(mem)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/canvas.py", line 426, in Redraw
    self.GetDiagram().Redraw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/diagram.py", line 42, in Redraw
    object.Draw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 1436, in Draw
    self.GetEventHandler().OnDraw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/pyenv-3.12.9/lib/python3.12/site-packages/wx/lib/ogl/basic.py", line 154, in OnDraw
    self._previousHandler.OnDraw(dc)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/shapes/UmlClass.py", line 150, in OnDraw
    self._startClipping(dc=dc, leftX=x, leftY=y)
  File "/Users/humberto.a.sanchez.ii/PycharmProjects/umlshapes-2779/src/umlshapes/shapes/UmlClass.py", line 509, in _startClipping
    dc.SetClippingRegion(x, y, w, h)
TypeError: DC.SetClippingRegion(): arguments did not match any overloaded call:
  overload 1: argument 1 has unexpected type 'float'
  overload 2: argument 1 has unexpected type 'float'
  overload 3: argument 1 has unexpected type 'float'

Process finished with exit code 133 (interrupted by signal 5:SIGTRAP)

[swt2c]

Those errors are not in wxPython code - you'll have to fix UmlClass.py separately.

[hasii2011]

You would think so. But that would depend on what Shape.GetX() Shape.GetY(), Shape.GetWidth() and Shape.GetHeight() are supposed to return.

In this case they are returning floats. But the DC.SetClippingRegions() expects integer values.

I assumed they would always be integer values. The documentation is unclear and a look at the code yields no type hints

What do you think the return type of those Shape methods is supposed to be?

[swt2c]

What do you think the return type of those Shape methods is supposed to be?

float, it seems: https://github.com/wxWidgets/Phoenix/blob/master/wx/lib/ogl/basic.py#L327

[hasii2011]

Really? So it is our expectation that when interacting with the wx.lib.ogl library and using its return values to interact with standard wx.DC API, we expect the developer to coerce all wx.lib.ogl return values.

That was actually my first approach (on a work around basis), but it quickly became unwieldly and looked "hackish".

But, if that is the final word, perhaps we should update the docstrings for these APIs and make it explicit (at least more than it is now).

[swt2c]

I mean, it's not great, but it's how this stuff worked before Python 3.10, it's just that Python silently truncated the integers when passing them to wxPython C++ APIs.

If we change wx.lib.ogl to return ints now, that would be an API change. Maybe one that makes sense, but it could break people's existing code.

[hasii2011]

Is that your final answer 😇 😇 😇 😇 😀 😀 😀 😀?

[swt2c]

Is that your final answer 😇 😇 😇 😇 😀 😀 😀 😀?

As final as anything is in this world. :-)