Rotate tool should only show front facing rings #169
Conversation
toloudis
requested review from
ShrimpCryptid and
frasercl
and removed request for
a team
| glm::vec4 discPlane(camFrame.vz, -glm::dot(camFrame.vz, axis.p)); | ||
|
|
There was a problem hiding this comment.
Could you break down what this line is doing? I'm assuming it's making a normal vector pointing towards the camera (to be the clipping plane's normal vector) but I'm not quite following how it's getting there.
Is camFrame.vz the vector pointing out/away from the camera, and the dot product is getting a magnitude along that vector...?
There was a problem hiding this comment.
This is standard geometry/linear algebra. The construct for a plane: a normal vector and a distance from the origin. The plane equation is Ax+By+Cz=D. camFrame.vz is the view direction which I am using as the normal to the plane. That provides the A,B,C. And the plane is positioned at (axis.p dot camFrame.vz) units away from the eye. That's D. axis.p is the world space center of the gizmo.
When you "dot" this plane vector with a point (x,y,z,1), you get a number that is positive or negative if the point is on either side of the plane. That's the equivalent to the value of Ax+By+Cz-D. if it's zero, the point is on the plane.
There was a problem hiding this comment.
...And the plane is positioned at (
axis.pdotcamFrame.vz) units away from the eye...
Clarification question, should this be units away from the origin?
There was a problem hiding this comment.
Yes, my mistake- it's in world space units from the origin, not from the eye.
| if (glm::dot(*clipPlane, glm::vec4(p0, 1.0)) > 0 && glm::dot(*clipPlane, glm::vec4(p1, 1.0)) > 0) { | ||
| graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code), | ||
| Gesture::Graphics::VertsCode(p1, color, opacity, code)); | ||
| } |
There was a problem hiding this comment.
Out of scope of this PR, but if you have a small number of segments or the segments are really large on screen, segments will be hidden if p0 is in front of the clip plane and p1 is not (or vice-versa). This might cause visual popping as the whole segment is suddenly rendered or not.
A nicer fix would be to move p0 or p1 to lie on the plane in that case, but it's more code and the bug is probably not visible in AGAVE. Is this current solution (discarding the whole line segment) the standard behavior when rendering with clipping planes?
There was a problem hiding this comment.
Yeah this is admittedly crude but it works if the circles use small enough segments. I'll refine it as needed for future usage. Currently there's nothing else that needs to clip, yet.
There was a problem hiding this comment.
It's also possible to use gpu clip planes and have this all done in the shader with good interpolation, but it would result in potentially splitting up the geometry into more draw calls. Currently the code can accumulate lots of line segments into a single draw call even if they are drawing many different things. And while things are still super cheap to draw right now, I didn't feel like routing all the data into the shader and splitting draw calls if they need different clip planes.
|
One more question, just as a check for a possible bug, if you're looking straight down an axis, will the ring that faces that axis be visible or clipped? Could be worth adding a slight offset to the plane just in case. |
It is very possible that it's clipped but in that case there is a white ring around the outside (always present, never clipped) which would rotate about the same axis so there is no loss of functionality. I would consider changing the test to be >= instead of > to account for when the ring is in the plane. |
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.4 to 1.15.6. - [Release notes](https://github.com/follow-redirects/follow-redirects/releases) - [Commits](follow-redirects/follow-redirects@v1.15.4...v1.15.6) --- updated-dependencies: - dependency-name: follow-redirects dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [webpack-dev-middleware](https://github.com/webpack/webpack-dev-middleware) from 5.3.3 to 5.3.4. - [Release notes](https://github.com/webpack/webpack-dev-middleware/releases) - [Changelog](https://github.com/webpack/webpack-dev-middleware/blob/v5.3.4/CHANGELOG.md) - [Commits](webpack/webpack-dev-middleware@v5.3.3...v5.3.4) --- updated-dependencies: - dependency-name: webpack-dev-middleware dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Fix: clear selection buffer, and zooming in ortho mode
|
oops I merged in the wrong order and now this PR has extra changes in it |
| @@ -126,6 +126,7 @@ cameraManipulationDolly(const glm::vec2 viewportSize, | |||
| camera.m_From += motion; | |||
| camera.m_Target += targetMotion; | |||
| camera.m_OrthoScale *= factor; | |||
| camera.Update(); | |||
There was a problem hiding this comment.
ignore; approved in previous PR and merged by accident into this one
| // it would save some computation but this is really not a bottleneck here. | ||
| if (selection) { | ||
| selection->clear(); | ||
| } |
There was a problem hiding this comment.
ignore; approved in previous PR and merged by accident into this one
| @@ -139,7 +139,7 @@ readTiffNumScenes(TIFF* tiff, const std::string& filepath) | |||
| LOG_WARNING << "Failed to read imagedescription of TIFF: '" << filepath << "'; interpreting as single channel."; | |||
| } | |||
|
|
|||
| std::string simagedescription = trim(imagedescription); | |||
| std::string simagedescription = trim(imagedescription ? imagedescription : ""); | |||
There was a problem hiding this comment.
ignore; approved in previous PR and merged by accident into this one
There was a problem hiding this comment.
Looks good! Your answers to @ShrimpCryptid's clarifying questions were very helpful to my understanding as well.
The rotate tool was drawing complete axis aligned rings which could create confusing interactions because you can't tell which part of the ring is facing forward.
This code change only shows the part of the ring that is forward facing and only allows drag interactions with that part of the ring. This makes axis constrained drag rotation much more intuitive.
Before:
After: