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

Fix Bounding Box download modal and node selection #5624

Merged
merged 19 commits into from
Jul 21, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jul 15, 2021

This PR fixes the Bounding Box download modal. It could not be rendered, because the context of the redux store was missing when using renderIndepentendly. I just incorporated this now into the bounding box tab, where the store context is available.

The node selection was too eager / worked for too far away nodes. See #5621. This is hopefully fixed via setting the far plane of the camera used to render the scene with the skeletons to 1. Thus all far away nodes should be clipped away and cannot be selected anymore.
But this needs some more testing.

URL of deployed dev instance (used for testing):

Steps to test:

  • Turn on the long-running jobs (jobsEnabled=true)
  • Open a tracing and create a bounding box in the bounding box tab in the right sidebar
  • Then use the download button of that bounding box
  • The export modal should open up without an error 🎉

  • Open a tracing (or use the same)
  • Select the skeleton tool and try to create nodes in every viewport by moving some slices forth and always creating nodes at the same position. This should no longer select the previously created node. For an example of how to test this in a viewport, see Unable to create nodes in YZ plane when too close to previous node #5621.
  • Please try this multiple times in a round-robin style in the viewports, as for me producing the bug wasn't always possible. It was kinda heisenbuggy.

Issues:


@philippotto
Copy link
Member

This is hopefully fixed via setting the far plane of the camera used to render the scene with the skeletons to 1. Thus all far away nodes should be clipped away and cannot be selected anymore.

Wouldn't it make more sense to use the same clipping distance which is used for actual rendering? That way, the user can only select if and only if the node is indeed visible to the user.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

I'm snatching the review from @daniel-wer (I hope this is alright).

I don't understand why the original renderToTexture function does not use the same clipping distance as the normal rendering. I'll investigate a bit now, since @MichaelBuessemeyer probably hasn't time for that before Thursday. Ping me if I'm interfering 🙈

@philippotto philippotto removed the request for review from daniel-wer July 20, 2021 15:29
@philippotto
Copy link
Member

Alright, I'm handing the review back to @daniel-wer 😅 The good news is that using the actual far value works well and should be exactly what the user should expect. The bad news is that we cannot simply use the actual far always, since this leads to weird rendering issues (the data planes are hidden then) for certain clippingDistance values. I think, the reason are imprecisions combined with a plane which is exactly shifted by the far value. I tried to circumvent it by adding Number.EPSILON to the far value, but this didn't have an effect (0.001 did, though, but this seemed to hacky to me). That's why, I only set the far value when renderToTexture is called with withFarClipping (as @MichaelBuessemeyer already built). Since the data planes shouldn't be rendered, anyway, when picking a node, the above problem is irrelevant.

@daniel-wer Feel free to review the code and test again :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Code mostly lgtm, just some smaller clarifications 👍

The node selection in the viewports works very well. However, the node selection in the 3D view seems to be broken for me.
I wasn't able to test the bounding box export, online, but if you tested it successfully, it should be good to go :)

@philippotto
Copy link
Member

However, the node selection in the 3D view seems to be broken for me.

very good catch! I fixed it now :)

@daniel-wer
Copy link
Member

Code LGTM, I'll do a final test once the CI build went through.

@philippotto
Copy link
Member

Code LGTM, I'll do a final test once the CI build went through.

cool! you can test it now here: https://panelcontentandnewcontrolsfollowup.webknossos.xyz/annotations/Explorational/60f821970100008a00e04803#459,516,512,0,2.000,1

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Awesome 👍

@philippotto philippotto enabled auto-merge (squash) July 21, 2021 13:41
@philippotto philippotto merged commit e1d38d8 into master Jul 21, 2021
@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Jul 21, 2021

Thanks, for pushing this PR without me :)

@fm3 fm3 deleted the panel-content-and-new-controls-followup branch May 24, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants