CompuCell3D / cc3d-player5

CC3D Player 5
GNU Lesser General Public License v3.0
4 stars 4 forks source link

Release/4.6.0 #66

Closed maciekswat closed 4 months ago

maciekswat commented 5 months ago

This is the release PR - all changes have been reviewed.

The only thing that requires a review is the improved handling of screenshots - in particular, a detection if the user tries to insert screenshot configuration that already exists:

https://github.com/CompuCell3D/cc3d-player5/commit/2f2653ddd3c8442c96ffc104c150b1c7f9a7df93

fyffep commented 4 months ago

@maciekswat I found a bug in this version.

  1. Open CC3D with a fresh simulation.
  2. Press Step
  3. Click the camera icon
    Traceback (most recent call last):
    File "C:\ProgramData\miniconda3\envs\cc3d_4413_310\lib\site-packages\cc3d\player5\Graphics\GraphicsFrameWidget.py", line 666, in add_screenshot_conf
    self.qvtkWidget.add_screenshot_config(screenshot_manager=tvw.screenshotManager)
    File "C:\ProgramData\miniconda3\envs\cc3d_4413_310\lib\site-packages\cc3d\core\GraphicsUtils\GraphicsFrame.py", line 557, in add_screenshot_config
    screenshot_manager.add_2d_screenshot(self.field_name, field_type, plane,
    File "C:\ProgramData\miniconda3\envs\cc3d_4413_310\lib\site-packages\cc3d\player5\Plugins\ViewManagerPlugins\ScreenshotManager.py", line 136, in add_2d_screenshot
    self.update_screenshot_container(scr_data=scr_data, _camera=_camera, metadata=metadata)
    File "C:\ProgramData\miniconda3\envs\cc3d_4413_310\lib\site-packages\cc3d\player5\Plugins\ViewManagerPlugins\ScreenshotManager.py", line 174, in update_screenshot_container
    self.screenshot_config_counter += 1
    AttributeError: 'ScreenshotManager' object has no attribute 'screenshot_config_counter'
    QThread: Destroyed while thread is still running
maciekswat commented 4 months ago

@fyffep this is interesting. I repeated the steps you listed and I had no such issues. I am wondering if this might be related to stale scripts that somehow did not get updated in your repo? I checked it on both Windows and OSX. Could you do one more check on your computer by installing CC3D using automated installer - let's make sure we check it now rather than after the release

fyffep commented 4 months ago

@fyffep this is interesting. I repeated the steps you listed and I had no such issues. I am wondering if this might be related to stale scripts that somehow did not get updated in your repo? I checked it on both Windows and OSX. Could you do one more check on your computer by installing CC3D using automated installer - let's make sure we check it now rather than after the release

I think this is expected. If I go to the Files Changed tab on this PR, there is only one usage of the variable. Line 174:

 self.screenshot_config_counter += 1

It has to be set to 0 somewhere. Maybe your changes were not committed?

maciekswat commented 4 months ago

@fyffep

@fyffep this is interesting. I repeated the steps you listed and I had no such issues. I am wondering if this might be related to stale scripts that somehow did not get updated in your repo? I checked it on both Windows and OSX. Could you do one more check on your computer by installing CC3D using automated installer - let's make sure we check it now rather than after the release

I think this is expected. If I go to the Files Changed tab on this PR, there is only one usage of the variable. Line 174:

 self.screenshot_config_counter += 1

It has to be set to 0 somewhere. Maybe your changes were not committed? @fyffep

Indeed you are right, there is just one reference but this is a derived class so the definition of this member variable is in the parent class:

class ScreenshotManagerCore(object):
    def __init__(self):

        self.screenshotDataDict = {}
...
        self.padding = 4
        self.screenshot_config_counter = 0
maciekswat commented 4 months ago

All issues raised have been resolved. Merging the PR