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

Add SetupAxisTicks for configuring custom ticks #55

Merged
merged 6 commits into from
Jan 16, 2025

Conversation

HeresJ0nny
Copy link
Contributor

Add SetupAxisTicks to ImPlot3D

This pull request introduces the SetupAxisTicks feature to ImPlot3D, bringing functionality similar to ImPlot's implementation. Key changes and improvements include:

1. Addition of SetupAxisTicks:
A new function SetupAxisTicks has been implemented for ImPlot3D, closely mirroring the behavior in ImPlot.
The only exception is the handling of the ShowDefaultTicks boolean, which is automatically set when using SetupAxisTicks. Future updates could expand its configurability if needed.

2. Axis Reset Refactoring:
The axis reset logic has been moved into BeginPlot for better initialization and consistency.
A Reset() method has been added to the axis struct, making it easier to manage axis state during plot setup.

3. Custom Tick Rendering Improvements:
Added checks in the rendering logic for grid lines, tick markers, and tick labels to ensure that custom ticks outside the axis range are not rendered. This ensures proper handling and prevents visual inconsistencies.

@brenocq brenocq self-requested a review January 14, 2025 16:35
@brenocq brenocq added type:feat New feature or request prio:medium Medium priority status:review The task is under review labels Jan 14, 2025
@brenocq
Copy link
Owner

brenocq commented Jan 14, 2025

Thanks for the addition @HeresJ0nny! I personally didn't know about this SetupAxisTicks from ImPlot, very handy function!

I pushed a commit to fix the code syntax. We use clang-format in ImPlot3D to ensure consistent code syntax.

  1. Addition of SetupAxisTicks:
    A new function SetupAxisTicks has been implemented for ImPlot3D, closely mirroring the behavior in ImPlot.
    The only exception is the handling of the ShowDefaultTicks boolean, which is automatically set when using SetupAxisTicks. Future updates could expand its configurability if needed.

I added the keep_default to the argument list to follow the same function signature as ImPlot. Let me know if there was a particular reason for you to have implemented it without keep_default (maybe I'm missing something).

  1. Axis Reset Refactoring:
    The axis reset logic has been moved into BeginPlot for better initialization and consistency.
    A Reset() method has been added to the axis struct, making it easier to manage axis state during plot setup.

Awesome!

  1. Custom Tick Rendering Improvements:
    Added checks in the rendering logic for grid lines, tick markers, and tick labels to ensure that custom ticks outside the axis range are not rendered. This ensures proper handling and prevents visual inconsistencies.

Thank you!!


Finally, it would be great to add a demo using SetupAxisTicks to the implot3d_demo.cpp. After having the demo in place I think it will be perfect for merge :)

We can use this demo from ImPlot as inspiration

void Demo_TickLabels()  {
    static bool custom_fmt    = true;
    static bool custom_ticks  = false;
    static bool custom_labels = true;
    ImGui::Checkbox("Show Custom Format", &custom_fmt);
    ImGui::SameLine();
    ImGui::Checkbox("Show Custom Ticks", &custom_ticks);
    if (custom_ticks) {
        ImGui::SameLine();
        ImGui::Checkbox("Show Custom Labels", &custom_labels);
    }
    const double pi = 3.14;
    const char* pi_str[] = {"PI"};
    static double yticks[] = {100,300,700,900};
    static const char*  ylabels[] = {"One","Three","Seven","Nine"};
    static double yticks_aux[] = {0.2,0.4,0.6};
    static const char* ylabels_aux[] = {"A","B","C","D","E","F"};
    if (ImPlot::BeginPlot("##Ticks")) {
        ImPlot::SetupAxesLimits(2.5,5,0,1000);
        ImPlot::SetupAxis(ImAxis_Y2, nullptr, ImPlotAxisFlags_AuxDefault);
        ImPlot::SetupAxis(ImAxis_Y3, nullptr, ImPlotAxisFlags_AuxDefault);
        if (custom_fmt) {
            ImPlot::SetupAxisFormat(ImAxis_X1, "%g ms");
            ImPlot::SetupAxisFormat(ImAxis_Y1, MetricFormatter, (void*)"Hz");
            ImPlot::SetupAxisFormat(ImAxis_Y2, "%g dB");
            ImPlot::SetupAxisFormat(ImAxis_Y3, MetricFormatter, (void*)"m");
        }
        if (custom_ticks) {
            ImPlot::SetupAxisTicks(ImAxis_X1, &pi,1,custom_labels ? pi_str : nullptr, true);
            ImPlot::SetupAxisTicks(ImAxis_Y1, yticks, 4, custom_labels ? ylabels : nullptr, false);
            ImPlot::SetupAxisTicks(ImAxis_Y2, yticks_aux, 3, custom_labels ? ylabels_aux : nullptr, false);
            ImPlot::SetupAxisTicks(ImAxis_Y3, 0, 1, 6, custom_labels ? ylabels_aux : nullptr, false);
        }
        ImPlot::EndPlot();
    }
}

@HeresJ0nny
Copy link
Contributor Author

Update: Enhanced SetupAxisTicks and Demo Integration

1. SetupAxisTicks Enhancements:
The SetupAxisTicks implementation for ImPlot3D now includes both function variants for consistency with the ImPlot API.

2. Template Function Addition:
Introduced the FillRange template function from ImPlot. This addition enhances code clarity by sharing functionality between ImPlot and ImPlot3D, facilitating easier identification of common features. This could be particularly relevant in the future if shared functionality between ImPlot and ImPlot3D is consolidated into a common utility.

3. Demo Integration:
Added a new demo to implot3d_demo.cpp, based on ImPlot's Demo_TickLabels.
The demo has been modified to focus on custom tick functionality and includes ImAxis3D_Z for completeness.

I added the keep_default to the argument list to follow the same function signature as ImPlot. Let me know if there was a particular reason for you to have implemented it without keep_default (maybe I'm missing something).

Initially, I had omitted keep_default due to concerns about overlapping tick labels when using custom ticks, as the current implementation does not check for sufficient space between labels. However, its inclusion of course ensures full parity with ImPlot.

I pushed a commit to fix the code syntax. We use clang-format in ImPlot3D to ensure consistent code syntax.

I want to apologize for not adhering to the clang-format guidelines in my earlier contribution. It was an oversight on my part, and I appreciate the adjustments that were made to ensure the codebase remains consistent and well-structured. This time, I made sure to review the formatting carefully, and I hope everything is now correct and meets your expectations. Thank you!

Copy link
Owner

@brenocq brenocq left a comment

Choose a reason for hiding this comment

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

Hi @HeresJ0nny! Thanks for implementing the demo :)

Since you implemented the range variant of SetupAxisTicks (thanks for that!), I also added it to the demo.

I removed the TempDouble1 and TempDouble2 from the context since they are not really needed at this point.

Initially, I had omitted keep_default due to concerns about overlapping tick labels when using custom ticks, as the current implementation does not check for sufficient space between labels. However, its inclusion of course ensures full parity with ImPlot.

Oooh I see! Year there is still some work needed to get the axes ticks looking good...

I want to apologize for not adhering to the clang-format guidelines in my earlier contribution.

No problemo :)

I think this PR is ready for merge, let me know if you agree with my changes and we can merge it!

@HeresJ0nny
Copy link
Contributor Author

Thank you for your updates and feedback! Adding the range variant of SetupAxisTicks to the demo is a great enhancement—thank you for including it.

I also appreciate you removing TempDouble1 and TempDouble2. I must admit, I had overlooked them during my initial implementation, so I’m grateful you caught that detail.

I’m fully satisfied with the changes you’ve made, and I believe the PR is now in excellent shape for merging. Thank you once again for your collaboration and support throughout this process!

Best regards!

@brenocq brenocq changed the base branch from main to dev January 16, 2025 15:19
@brenocq brenocq merged commit 69ee921 into brenocq:dev Jan 16, 2025
@brenocq brenocq added status:done Task completed successfully and removed status:review The task is under review labels Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:medium Medium priority status:done Task completed successfully type:feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants