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.

Sign up for GitHub

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

Rotate tool should only show front facing rings #169

Merged
merged 7 commits into from
Apr 1, 2024
Merged

Rotate tool should only show front facing rings #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
17ee71d
allow circles to be clipped against a plane
toloudis Mar 18, 2024
9f64663
fix tiff with empty imagedescription
toloudis Mar 20, 2024
007b9a3
Bump follow-redirects from 1.15.4 to 1.15.6 in /webclient
dependabot[bot] Mar 16, 2024
749f062
Bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /webclient
dependabot[bot] Mar 22, 2024
2e529af
needed to clear the selection buffer more often
toloudis Mar 23, 2024
86de551
fix ortho zoom
toloudis Mar 23, 2024
88f604c
Merge pull request #172 from allen-cell-animated/fix/clear-selection
toloudis Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions renderlib/CCamera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ cameraManipulationDolly(const glm::vec2 viewportSize,
camera.m_From += motion;
camera.m_Target += targetMotion;
camera.m_OrthoScale *= factor;
camera.Update();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore; approved in previous PR and merged by accident into this one


// Consume gesture on button release
Gesture::Input::reset(button);
Expand Down
8 changes: 5 additions & 3 deletions renderlib/RotateTool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)

gesture.graphics.addCommand(GL_LINES);

glm::vec4 discPlane(camFrame.vz, -glm::dot(camFrame.vz, axis.p));

Comment on lines +264 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

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...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

...And the plane is positioned at (axis.p dot camFrame.vz) units away from the eye...

Clarification question, should this be units away from the origin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my mistake- it's in world space units from the origin, not from the eye.

float axisscale = scale * 0.85f;
// Draw the x ring in yz plane
{
Expand All @@ -269,7 +271,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateX) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vy * axisscale, axis.l.vz * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vy * axisscale, axis.l.vz * axisscale, 48, color, 1, code, &discPlane);
}
// Draw the y ring in xz plane
{
Expand All @@ -278,7 +280,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateY) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vz * axisscale, axis.l.vx * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vz * axisscale, axis.l.vx * axisscale, 48, color, 1, code, &discPlane);
}
// Draw the z ring in xy plane
{
Expand All @@ -287,7 +289,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateZ) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vx * axisscale, axis.l.vy * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vx * axisscale, axis.l.vy * axisscale, 48, color, 1, code, &discPlane);
}

// Draw the camera-axis rotation manipulator as a circle always facing the view
Expand Down
2 changes: 0 additions & 2 deletions renderlib/ViewerWindow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class ViewerWindow
m_activeTool = (tool ? tool : &m_defaultTool);

// clear out the buffer once.
// we could alternatively flag this for clearing on the next update.
// see in update() where we check for no vertices.
if (!tool) {
m_selection.clear();
}
Expand Down
22 changes: 18 additions & 4 deletions renderlib/gesture/gesture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,18 @@ class ScopedGlVertexBuffer
};

void
Gesture::Graphics::draw(SceneView& sceneView, const SelectionBuffer* selection)
Gesture::Graphics::draw(SceneView& sceneView, SelectionBuffer* selection)
{
// Gesture draw spans across the entire window and it is not restricted to a single
// viewport.
if (this->verts.empty()) {
clearCommands();

// TODO: do this clear only once if verts empty on consecutive frames?
// it would save some computation but this is really not a bottleneck here.
if (selection) {
selection->clear();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore; approved in previous PR and merged by accident into this one

return;
}

Expand Down Expand Up @@ -616,7 +622,8 @@ Gesture::drawCircle(glm::vec3 center,
uint32_t numSegments,
glm::vec3 color,
float opacity,
uint32_t code)
uint32_t code,
glm::vec4* clipPlane)
{
for (int i = 0; i < numSegments; ++i) {
float t0 = float(i) / float(numSegments);
Expand All @@ -628,8 +635,15 @@ Gesture::drawCircle(glm::vec3 center,
glm::vec3 p0 = center + xaxis * cosf(theta0) + yaxis * sinf(theta0);
glm::vec3 p1 = center + xaxis * cosf(theta1) + yaxis * sinf(theta1);

graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code),
Gesture::Graphics::VertsCode(p1, color, opacity, code));
if (clipPlane) {
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));
}
Comment on lines +639 to +642
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} else {
graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code),
Gesture::Graphics::VertsCode(p1, color, opacity, code));
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions renderlib/gesture/gesture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ struct Gesture

// Gesture draw, called once per window update (frame) when the GUI draw commands
// had been described in full.
void draw(struct SceneView& sceneView, const struct SelectionBuffer* selection);
void draw(struct SceneView& sceneView, struct SelectionBuffer* selection);

// Pick a GUI element using the cursor position in Input.
// Return a valid GUI selection code, SelectionBuffer::k_noSelectionCode
Expand All @@ -424,7 +424,8 @@ struct Gesture
uint32_t numSegments,
glm::vec3 color,
float opacity,
uint32_t code);
uint32_t code,
glm::vec4* clipPlane = nullptr);

// does not draw a flat base
void drawCone(glm::vec3 base,
Expand Down
4 changes: 2 additions & 2 deletions renderlib/io/FileReaderTIFF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 : "");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore; approved in previous PR and merged by accident into this one


// check for plain tiff with ImageJ imagedescription:
if (startsWith(simagedescription, "ImageJ=")) {
Expand Down Expand Up @@ -246,7 +246,7 @@ readTiffDimensions(TIFF* tiff, const std::string filepath, VolumeDimensions& dim
std::vector<std::string> channelNames;
std::string dimensionOrder = "XYCZT";

std::string simagedescription = trim(imagedescription);
std::string simagedescription = trim(imagedescription ? imagedescription : "");

// check for plain tiff with ImageJ imagedescription:
if (startsWith(simagedescription, "ImageJ=")) {
Expand Down
12 changes: 6 additions & 6 deletions webclient/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading