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

Migrate from obselete QGLWidget to QOpenGLWidget #35104

Merged

Conversation

martyngigg
Copy link
Member

@martyngigg martyngigg commented Jan 27, 2023

Description of work.

QGLWidget has not been formally deprecated but there are several issues with it and the docs
mark it has obselete.

On macOS it also seems impossible to correct deal with moving an OpenGL window between
monitors with different pixel densities, while the new widget seems to deal with it seamlessly.
We have removed almost all glViewport calls as the base widget now handles this for us.

It has been necessary to set the default OpenGL profile to compatability mode as the current
code uses older OpenGL api calls. Future work will need to tackle this issue as it is outside of
the scope. A note has been added to the maintenance board.

Provides an alternate and better solution to #35058.

To test:

This needs to be tested on a mac with retina screen and an additional lower-resolution screen connected.
Use the following script to determine this information:

from qtpy.QtWidgets import QApplication
for screen in QApplication.screens():
    print(screen.name())
    print(screen.devicePixelRatio())

It should print 2.0 for a retina display and 1.0 for a standard external screen.
If all values are the same then the test of moving the window between screens will not be valid.

All tests will need a workspace with an instrument. Use the following script to create a test data set from
the training course data:

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

SXD23767 = Load(Filename='SXD23767.raw', LoadMonitors='Exclude')
peaksws = Load('peaks_qLab.nxs')
FindUBUsingFFT(PeaksWorkspace='peaksws', MinD=0.8, MaxD=10)
IndexPeaks(peaksws, Tolerance=0.05)

1. Open on Retina Screen

  • Move workbench to the retina screen
  • Click show instrument and check the instrument image is full size in the viewport
  • Change to one of the unwrapped views, i.e CylinderY and the image should still be fullsize in the viewport

2. Open on the Standard Def Screen

  • Close any instrument view windows
  • Move workbench to the standard definition screen
  • Click show instrument and check the instrument image is full size in the viewport
  • Change to one of the unwrapped views, i.e CylinderY and the image should still be fullsize in the viewport

3. Move Window Between Screens

  • Close any instrument view windows
  • Click show instrument and check the instrument image is full size in the viewport
  • Drag the window to the retina screen and check that one the drag is complete the image is still full size in the viewport
  • Drag the window back and check again.
  • Do the above for the unwrapped view too.

Zooming

  • When on the Render tab use the mouse to select a rectangle and ensure the zoom takes you to the expected place.

Overlay peaks workspace

  • Close an instrument view windows
  • Load the SXD data set from the training course data
  • Also load the peaks_qLab.nxs from the training course data
  • Show the instrument on the data workspace
  • Drag and drop the peaks workspace onto the instrument
  • In the "Display Settings" alternate between "Use OpenGL" checked and unchecked
  • The peaks should be in the same positions.

Pick Tab

  • Move to the Pick tab
  • Move the mouse over the display and check the miniplot updates with the spectrum values
  • Move the mouse over peaks that are labeled and check the miniplot displays the HKL values on the miniplot
  • Select the peak picking tool
    • click on a peak on the instrument
    • click at the top of the peak that appears frozen in the miniplot and a new table workspace should have the expected peak values (check the values match those with OpenGL disabled)

Fixes #34081


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Are the release notes saved in a separate file, using Issue or PR number for file name and in the correct location?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

@martyngigg martyngigg changed the title Qglwidget to qopenglwidget Migrate from obselete QGLWidget to QOpenGLWidget Jan 27, 2023
@martyngigg martyngigg added the ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS label Jan 27, 2023
@martyngigg martyngigg added this to the Release 6.7 milestone Jan 27, 2023
Copy link
Contributor

@DanielMurphy22 DanielMurphy22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main instrument view shrink when using a retina + external low resolution screen on MacOS is fixed!
The following remaining problems exist on just the retina, even without the second screen:

  • On an unwrapped view like Cylindrical Y , the Zoombox does not always appear. It also doesn't zoom in on the area you selected. On the normal low res screen zooming works fine. Zooming also works fine when you disable opengl rendering.
  • Scrolling to zoom on Full 3D doesn't zoom on the cursor.
  • Overlaid peaks appear in a different place whether opengl is turned on or not. Again on the normal low res screen this works fine.

Also, as we are now in the OpenGL compatibility mode, we get this message on launching workbench:

An OpenGL surfcace format was requested that is either not version 3.2 or higher or a not Core Profile.
Chromium on macOS will fall back to software rendering in this case.
Hardware acceleration and features such as WebGL will not be available.

@DanielMurphy22 DanielMurphy22 self-assigned this Jan 30, 2023
@DanielMurphy22 DanielMurphy22 added the macOS Only The issues related to macOS only. label Jan 30, 2023
Classes are no longer referenced across the code
and now only serve as a maintenance burden and
point of confusion
The flag was flipped to false once but is
no longer used.
Make it clear that these methods can
be updated without affecting other classes
QGLWidget has not been formally deprecated but
there are several issues with it and the docs
mark it has obselete.
On macOS it also seems impossible to correct
deal with moving an OpenGL window between
monitors with different pixel densities, while
the new widget seems to deal with it seamlessly.
We have removed almost all glViewport calls as
the base widget now handles this for us.
Remove references to partially implemented
perspective projection type. It has never been
used and as the code stated it did not fully work
as intended. It's just debt by this point.
By default the viewport/window fill the entire device pixel
space. For high-density pixel displays this gives
increased possibility for drawing at a finer level.
Our 2D shape drawing code assumes logical pixels for all
transforms that are computed so we must set the viewport/window
transformations to account for the possibilty that widget
width in logical coordinates does not match the width in
devce pixels.
The view is marked for update at the appropriate times
so this is no longer required. For High-DPI displays
it was always giving false anyway due to the image
and widget widths given in device and logical coordinates
respectively.
This infomation is then available if the image dimensions
need converting back to logical pixel coordinates.
@martyngigg martyngigg marked this pull request as ready for review February 2, 2023 08:21
@martyngigg
Copy link
Member Author

The main instrument view shrink when using a retina + external low resolution screen on MacOS is fixed! The following remaining problems exist on just the retina, even without the second screen:

* On an unwrapped view like `Cylindrical Y` , the Zoombox does not always appear. It also doesn't zoom in on the area you selected. On the normal low res screen zooming works fine. Zooming also works fine when you disable opengl rendering.

* Scrolling to zoom on Full 3D doesn't zoom on the cursor.

* Overlaid peaks appear in a different place whether opengl is turned on or not. Again on the normal low res screen this works fine.

I think I have fixed all of the☝🏻points.

Also, as we are now in the OpenGL compatibility mode, we get this message on launching workbench:

An OpenGL surfcace format was requested that is either not version 3.2 or higher or a not Core Profile.
Chromium on macOS will fall back to software rendering in this case.
Hardware acceleration and features such as WebGL will not be available.

I can't decide whether we should hide this or not. It's macOS only and users will not see it as they will not a get a terminal when launching it via the GUI. I like the idea of it being in the face of developers that there is a deprecation issue so perhaps we just need to put a note in the developer docs that this is a known warning - what do you think?

@DanielMurphy22
Copy link
Contributor

DanielMurphy22 commented Feb 2, 2023

Yes I think I agree that keeping the deprecation warning is good. Whether it's in the dev-docs or a code comment, please include the full error. If I was going looking for it, I'd probably do a git grep and that should help find your note!

@DanielMurphy22
Copy link
Contributor

Can you add the full warning message to the issue you raised to move away from this compatibility mode?

In only affects mac developers and will not be visible to
users. We want to keep it visible to developers as a reminder
that deprecated OpenGL functionality is being used.
Copy link
Contributor

@DanielMurphy22 DanielMurphy22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works a treat! Great to have the up to date QOpenGL widget!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS macOS Only The issues related to macOS only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument view window shrink
3 participants