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

Replace Dropzone VTKActor by IMGUI #1990

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Ni-g-3l
Copy link
Contributor

@Ni-g-3l Ni-g-3l commented Feb 15, 2025

Hello !

I try to replace DropZone Actor by a IMGUI window. I'm not ImGUI expert if you see a better way to do it do not hesitate to comment.

image

Linked issue : #1975

@mwestphal
Copy link
Contributor

Do you think you could put an outline around the text, like its done for the filename ?

@mwestphal
Copy link
Contributor

Other than that, its looking good! Dont hesitate to share such screenshot on discord :)

@mwestphal
Copy link
Contributor

However, how can I update tests in order to match the new dropzone ?

You just need to update the baselines

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 15, 2025

Sorry I don't understand ? Where I can find the example to put outline on text ? because my update on DropText look very similar at what it is done for FileName

@mwestphal
Copy link
Contributor

I think its handled by SetupNextWindow

@mwestphal
Copy link
Contributor

mwestphal commented Feb 15, 2025

I mean this:
a

@snoyer
Copy link
Contributor

snoyer commented Feb 16, 2025

@Ni-g-3l I would suggest trying absurdly high values for tickThickness and tickLength in order to highlight accuracy/precision issues in the drawing logic.

For example, using:

    constexpr float tickThickness = 25.0f;
    constexpr float tickLength = 40.0f;

this PR's ImGui code shows:
image
while the previous VTK code shows:
Screenshot From 2025-02-16 09-25-07

The key point here is that the previous code was drawing rectangles and the new code is drawing thick lines so there's going to be some ±thickness/2 offsets to account for somewhere somehow

(also check to make sure it all still works when resizing the window)

possible fix/improvement
diff --git a/vtkext/private/module/vtkF3DImguiActor.cxx b/vtkext/private/module/vtkF3DImguiActor.cxx
index b54e941c..c386d537 100644
--- a/vtkext/private/module/vtkF3DImguiActor.cxx
+++ b/vtkext/private/module/vtkF3DImguiActor.cxx
@@ -299,8 +299,8 @@ void vtkF3DImguiActor::RenderDropZone()
     constexpr float tickLength = 10.0f;
 
     int dropzonePad = static_cast<int>(std::min(viewport->WorkSize.x, viewport->WorkSize.y) * 0.1);
-    int dropZoneW = viewport->WorkSize.x - dropzonePad;
-    int dropZoneH = viewport->WorkSize.y - dropzonePad;
+    int dropZoneW = viewport->WorkSize.x - dropzonePad * 2;
+    int dropZoneH = viewport->WorkSize.y - dropzonePad * 2;
 
     ::SetupNextWindow(ImVec2(0, 0), viewport->WorkSize);
     ImGui::SetNextWindowBgAlpha(0.f);
@@ -312,7 +312,7 @@ void vtkF3DImguiActor::RenderDropZone()
     ImDrawList* draw_list = ImGui::GetWindowDrawList();
 
     ImVec2 p0(dropzonePad, dropzonePad);
-    ImVec2 p1(dropZoneW, dropZoneH);
+    ImVec2 p1(dropzonePad + dropZoneW, dropzonePad + dropZoneH);
     int tickNumberW = static_cast<int>(std::ceil(dropZoneW / (tickLength * 2.0f)));
     int tickNumberH = static_cast<int>(std::ceil(dropZoneH / (tickLength * 2.0f)));
     double tickSpaceW =
@@ -323,22 +323,24 @@ void vtkF3DImguiActor::RenderDropZone()
     // Draw top and bottom line
     for (float x = p0.x; x < p1.x; x += tickLength + tickSpaceW)
     {
-      draw_list->AddLine(ImVec2(x, p0.y), ImVec2(x + tickLength, p0.y), color, tickThickness);
-      draw_list->AddLine(ImVec2(x, p1.y), ImVec2(x + tickLength, p1.y), color, tickThickness);
+      const float y0 = p0.y + tickThickness / 2.f;
+      const float y1 = p1.y - tickThickness / 2.f;
+      draw_list->AddLine(ImVec2(x, y0), ImVec2(x + tickLength, y0), color, tickThickness);
+      draw_list->AddLine(ImVec2(x, y1), ImVec2(x + tickLength, y1), color, tickThickness);
     }
 
     // Draw left and right line
     for (float y = p0.y; y < p1.y; y += tickLength + tickSpaceH)
     {
-      draw_list->AddLine(ImVec2(p0.x, y), ImVec2(p0.x, y + tickLength), color, tickThickness);
-      draw_list->AddLine(ImVec2(p1.x, y), ImVec2(p1.x, y + tickLength), color, tickThickness);
+      const float x0 = p0.x + tickThickness / 2.f;
+      const float x1 = p1.x - tickThickness / 2.f;
+      draw_list->AddLine(ImVec2(x0, y), ImVec2(x0, y + tickLength), color, tickThickness);
+      draw_list->AddLine(ImVec2(x1, y), ImVec2(x1, y + tickLength), color, tickThickness);
     }
 
     ImGui::End();
 
     ImVec2 dropTextSize = ImGui::CalcTextSize(this->DropText.c_str());
-    dropTextSize.x += 2.f * ImGui::GetStyle().WindowPadding.x;
-    dropTextSize.y += 2.f * ImGui::GetStyle().WindowPadding.y;
 
     ImGui::Begin("DropZoneText", nullptr, flags);
     ImGui::SetCursorPos(ImVec2(viewport->GetWorkCenter().x - 0.5f * dropTextSize.x,

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 16, 2025

I try to fix what you find, with my latest commit

image

image

image

image

I will fix the CICD tomorow

Copy link
Contributor

@snoyer snoyer left a comment

Choose a reason for hiding this comment

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

@Ni-g-3l I believe you forgot to double the padding at the beginning and ended up trying to compensate for that all along later

Comment on lines 323 to 348
double tickBBSizeW = tickLength + tickSpaceW;
double tickBBSizeH = tickLength + tickSpaceH;

// Draw top and bottom line
for (float x = p0.x - tickHalfThickness; x < p1.x - tickBBSizeW; x += tickBBSizeW)
{
draw_list->AddLine(ImVec2(x, p0.y), ImVec2(x + tickLength, p0.y), color, tickThickness);
draw_list->AddLine(ImVec2(x, p1.y), ImVec2(x + tickLength, p1.y), color, tickThickness);
}

draw_list->AddLine(ImVec2(p1.x - tickLength, p0.y), ImVec2(p1.x + tickHalfThickness, p0.y),
color, tickThickness);
draw_list->AddLine(ImVec2(p1.x - tickLength, p1.y), ImVec2(p1.x + tickHalfThickness, p1.y),
color, tickThickness);

// Draw left and right line
for (float y = p0.y - tickHalfThickness; y < p1.y - tickBBSizeH; y += tickBBSizeH)
{
draw_list->AddLine(ImVec2(p0.x, y), ImVec2(p0.x, y + tickLength), color, tickThickness);
draw_list->AddLine(ImVec2(p1.x, y), ImVec2(p1.x, y + tickLength), color, tickThickness);
}

draw_list->AddLine(ImVec2(p0.x, p1.y - tickLength), ImVec2(p0.x, p1.y + tickHalfThickness),
color, tickThickness);
draw_list->AddLine(ImVec2(p1.x, p1.y - tickLength), ImVec2(p1.x, p1.y + tickHalfThickness),
color, tickThickness);
Copy link
Contributor

Choose a reason for hiding this comment

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

the half-thickness offset you want is only to draw the lines inside the target rectangle rather that on it:
Screenshot From 2025-02-17 06-44-08
vs
Screenshot From 2025-02-17 06-44-31

So you just have to to shift the top/bottom lines down/up and the right/left lines left/right, no need to modify/extend the loops:

Suggested change
double tickBBSizeW = tickLength + tickSpaceW;
double tickBBSizeH = tickLength + tickSpaceH;
// Draw top and bottom line
for (float x = p0.x - tickHalfThickness; x < p1.x - tickBBSizeW; x += tickBBSizeW)
{
draw_list->AddLine(ImVec2(x, p0.y), ImVec2(x + tickLength, p0.y), color, tickThickness);
draw_list->AddLine(ImVec2(x, p1.y), ImVec2(x + tickLength, p1.y), color, tickThickness);
}
draw_list->AddLine(ImVec2(p1.x - tickLength, p0.y), ImVec2(p1.x + tickHalfThickness, p0.y),
color, tickThickness);
draw_list->AddLine(ImVec2(p1.x - tickLength, p1.y), ImVec2(p1.x + tickHalfThickness, p1.y),
color, tickThickness);
// Draw left and right line
for (float y = p0.y - tickHalfThickness; y < p1.y - tickBBSizeH; y += tickBBSizeH)
{
draw_list->AddLine(ImVec2(p0.x, y), ImVec2(p0.x, y + tickLength), color, tickThickness);
draw_list->AddLine(ImVec2(p1.x, y), ImVec2(p1.x, y + tickLength), color, tickThickness);
}
draw_list->AddLine(ImVec2(p0.x, p1.y - tickLength), ImVec2(p0.x, p1.y + tickHalfThickness),
color, tickThickness);
draw_list->AddLine(ImVec2(p1.x, p1.y - tickLength), ImVec2(p1.x, p1.y + tickHalfThickness),
color, tickThickness);
// Draw top and bottom line
for (float x = p0.x; x < p1.x; x += tickLength + tickSpaceW)
{
const float y0 = p0.y + tickThickness / 2.f;
const float y1 = p1.y - tickThickness / 2.f;
draw_list->AddLine(ImVec2(x, y0), ImVec2(x + tickLength, y0), color, tickThickness);
draw_list->AddLine(ImVec2(x, y1), ImVec2(x + tickLength, y1), color, tickThickness);
}
// Draw left and right line
for (float y = p0.y; y < p1.y; y += tickLength + tickSpaceH)
{
const float x0 = p0.x + tickThickness / 2.f;
const float x1 = p1.x - tickThickness / 2.f;
draw_list->AddLine(ImVec2(x0, y), ImVec2(x0, y + tickLength), color, tickThickness);
draw_list->AddLine(ImVec2(x1, y), ImVec2(x1, y + tickLength), color, tickThickness);
}

@mwestphal mwestphal linked an issue Mar 1, 2025 that may be closed by this pull request
@mwestphal
Copy link
Contributor

Hi @Ni-g-3l

Do you need help or review into order to move forward ?

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Mar 3, 2025

Hello ! Sorry I was quite busy last week ! I will try to take a look this week in order to provide healthy base for the future implementation of the new drop zone

@Ni-g-3l Ni-g-3l force-pushed the remove-dropzone-vtk-actor branch from c68a5cb to 4268995 Compare March 5, 2025 08:38
@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Mar 5, 2025

So I try all of your propositions @snoyer, but it wasn't worked as expected. If we want to ensure the dropZone rectangle is always correctly closed we have to draw the the last tick independantly from the loop.

The current implementation is looking like this :
image
image
image

The only problem that I can see is the different space length between the last tick of each line and the previous one.

Feel free to review and comment if you think I should try another implementation !

@snoyer
Copy link
Contributor

snoyer commented Mar 5, 2025

@Ni-g-3l I think you should refer to the original computations for the dashing (you may want to keep the comments too)

// padding is 10% of shortest side
int padding = static_cast<int>(std::min(vSize[0], vSize[1]) * 0.1);
int borderW = vSize[0] - 2 * padding;
int borderH = vSize[1] - 2 * padding;
// number of ticks assuming spacing == tickLength
int nTicksW = static_cast<int>(std::ceil(borderW / (tickLength * 2.0)));
int nTicksH = static_cast<int>(std::ceil(borderH / (tickLength * 2.0)));
// recompute uniform spacing
double spacingW = static_cast<double>(borderW - nTicksW * tickLength) / (nTicksW - 1);
double spacingH = static_cast<double>(borderH - nTicksH * tickLength) / (nTicksH - 1);

I believe your original version was only missing the offsetting to draw inside of the target rectangle instead of exactly on it (see this 2 images #1990 (comment)).

Here's a snippet that seem to work for me:

    const double tickSpaceW =
      static_cast<double>(dropZoneW - tickNumberW * tickLength) / (tickNumberW - 1);
    const double tickSpaceH =
      static_cast<double>(dropZoneH - tickNumberH * tickLength) / (tickNumberH - 1);

    ::SetupNextWindow(ImVec2(0, 0), viewport->WorkSize);
    ImGui::SetNextWindowBgAlpha(0.f);

    ImGuiWindowFlags flags = ImGuiWindowFlags_NoDecoration | ImGuiWindowFlags_NoSavedSettings |
      ImGuiWindowFlags_NoFocusOnAppearing | ImGuiWindowFlags_NoNav | ImGuiWindowFlags_NoMove;

    ImGui::Begin("DropZoneText", nullptr, flags);
    ImDrawList* draw_list = ImGui::GetWindowDrawList();

    const ImVec2 p0(dropzonePad, dropzonePad);
    const ImVec2 p1(dropzonePad + dropZoneW, dropzonePad + dropZoneH);

    // Draw top and bottom line
    for (float x = p0.x; x < p1.x; x += tickLength + tickSpaceW)
    {
      const float y0 = p0.y + tickThickness / 2.f;
      const float y1 = p1.y - tickThickness / 2.f;
      draw_list->AddLine(ImVec2(x, y0), ImVec2(x + tickLength, y0), color, tickThickness);
      draw_list->AddLine(ImVec2(x, y1), ImVec2(x + tickLength, y1), color, tickThickness);
    }
    // Draw left and right line
    for (float y = p0.y; y < p1.y; y += tickLength + tickSpaceH)
    {
      const float x0 = p0.x + tickThickness / 2.f;
      const float x1 = p1.x - tickThickness / 2.f;
      draw_list->AddLine(ImVec2(x0, y), ImVec2(x0, y + tickLength), color, tickThickness);
      draw_list->AddLine(ImVec2(x1, y), ImVec2(x1, y + tickLength), color, tickThickness);
    }

    {
      /* target rectangle for debugging ;) */
      constexpr ImU32 red = IM_COL32(255, 0, 0, 255);
      draw_list->AddLine(ImVec2(p0.x, p0.x), ImVec2(p1.x, p0.y), red, 1);
      draw_list->AddLine(ImVec2(p0.x, p1.y), ImVec2(p1.x, p1.y), red, 1);
      draw_list->AddLine(ImVec2(p0.x, p0.x), ImVec2(p0.x, p1.y), red, 1);
      draw_list->AddLine(ImVec2(p1.x, p0.x), ImVec2(p1.x, p1.y), red, 1);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert dropzone widget into ImGui widget
3 participants