search

allen-cell-animated / agave

Other
31 stars 5 forks source link

Fix: clear selection buffer, and zooming in ortho mode #172

Closed toloudis closed 3 months ago

toloudis commented 3 months ago

Est. time to review: <10 min

Three small bugfixes:

  1. in some situations the rotation gizmo was still leaving data in the selection buffer even after it was disabled. This fix ensures that the selection buffer is cleared more aggressively.
  2. When zooming in orthographic camera mode, and then using the rotation gizmo, the view would become corrupted by showing the volume at the wrong scale. Fixed by calling camera.Update() after zoom/dolly.
  3. Some tiff files have no imagedescription at all. Agave was crashing on those files and this change allows them to be loaded.

Also dependabot put in some minor stuff into package-lock.json which is basically irrelevant to this PR (and that code is not being run or tested yet)

meganrm commented 3 months ago

Est. time to review: <10 min

Three small bugfixes:

  1. in some situations the rotation gizmo was still leaving data in the selection buffer even after it was disabled. This fix ensures that the selection buffer is cleared more aggressively.
  2. When zooming in orthographic camera mode, and then using the rotation gizmo, the view would become corrupted by showing the volume at the wrong scale. Fixed by calling camera.Update() after zoom/dolly.
  3. Some tiff files have no imagedescription at all. Agave was crashing on those files and this change allows them to be loaded.

Also dependabot put in some minor stuff into package-lock.json which is basically irrelevant to this PR (and that code is not being run or tested yet)

Given this isn't a language I'm familiar with: problems 2 and 3 here seem really clear and I understand the fixes. the fix for problem 1 seems small too but I don't understand why those changes fixed the problem

toloudis commented 3 months ago

the fix for problem 1 seems small too but I don't understand why those changes fixed the problem

I didn't completely prove this but I think the clear() was being called when there is no current opengl context and so issuing GL commands to clear a gpu buffer would be either a no op or undefined behavior. Rather than call makeCurrent/doneCurrent to grab the opengl context, I opt to clear the buffer as part of the regular redraw cycle where we are doing other gl stuff.