Feat: PlotPrismLine #129

AhmedLilah · 2025-06-09T23:46:39Z

Closes #126

Adding the PlotPrismLine function: plots an N-Sided Prism around a line to make it better looking in 3D and avoid plain-line intersection

…ne to make it better looking in 3D and avoid plain-line intersection

…s.cpp

….cpp

AhmedLilah · 2025-06-19T21:20:00Z

@brenocq

Do you think you can take a look at this to get an initial idea of whether or not to continue in this direction?

brenocq

First of all, sorry for taking so long to review it; a personal problem came up. I left some reviews regarding the unspoken code standards that we are following.

I was thinking that maybe we could rename this method to PlotPrism, and there could be some ImPlot3DPrismFlags to choose between rendering a closed prism (which is useful is someone wants to render a square prism, pentagonal prism, hexagonal prism, etc) or a prism with open ends (which can be used to remove the line artifacts for advanced users). There could also be flags to define if the const T* xs, const T* ys, const T* zs define individual prisms or chained prisms.

The prism could still be defined by two points, just like you implemented (a few prism types below)

I'm not sure what the best arguments are for the PlotPrism method. We could also have multiple PlotPrism methods with different arguments, maybe a PlotCube, PlotPipe, etc. Just throwing ideas here, being able to plot different types of prisms with ImPlot3D sounds quite useful :)

It would be great to also add a demo plotting different types of prisms.

Let me know what you think!

brenocq · 2025-07-17T04:01:11Z

implot3d_items.cpp

@@ -37,6 +38,7 @@

 #ifndef IMGUI_DEFINE_MATH_OPERATORS
 #define IMGUI_DEFINE_MATH_OPERATORS
+#include <type_traits>


In ImPlot3D/ImPlot/ImGui, we don't use C++ containers (I think it is mainly to avoid the compilation/performance overhead, but I'm not sure)

This is not for container types it's a type trait to use in concepts to limit the types for type safety.

but, we could do without.

Oh sorry! Yes, you're right. What I mean is trying to stick to the includes already present in ImGui/ImPlot as much as possible to avoid increasing compilation time with C/C++ libraries.

brenocq · 2025-07-17T04:03:18Z

implot3d_items.cpp

+//[Section] PlotPrismLine
+//-----------------------------------------------------------------------------
+
+template<typename T> requires(std::is_floating_point_v<T>) 


Awesome to see you using C++20! For now, since ImPlot doesn't use C++20 and we haven't used it in ImPlot3D yet, it would be nice to stick to C++17 features so people with older compilers can continue compiling ImPlot3D without problems.

(Note that the CMakeLists.txt specifies C++17, so this code wouldn't compile)

ok, i'll take it off it's not really nessary.

It was an attempt to code deffensively to prevent someone from using the wrong data type

I see! The CALL_INSTANTIATE_FOR_NUMERIC_TYPES macro takes care of only creating instances for the valid types; you can read more about it in [SECTION] Template instantiation utility.

brenocq · 2025-07-17T04:06:30Z

implot3d_items.cpp

+	auto quad_xs = (T*)malloc(count * sides * 4 * sizeof(T));
+        auto quad_ys = (T*)malloc(count * sides * 4 * sizeof(T));
+        auto quad_zs = (T*)malloc(count * sides * 4 * sizeof(T));


malloc is a bit error-prone. It would be better to use ImVector<T> when possible.

great, didn't know the ImGui had a dynamic array.

will look into it and change the code accordingly.

brenocq · 2025-07-17T04:08:59Z

implot3d_items.cpp

+        free(octagonPoints1);
+        free(octagonPoints2);


It would be nice to run clang-format before each commit! Most editors can run clang-format when the file is saved. Alternatively, you could also have a pre-commit hook running clang-format if you prefer.

will look into it.

although I think if i use ImVector that wouldn't be neccessary.

brenocq · 2025-07-17T04:15:04Z

implot3d_items.cpp

+//-----------------------------------------------------------------------------
+
+template<typename T> requires(std::is_floating_point_v<T>) 
+struct Vec3 {


It is a bit counterintuitive, but the ImPlot3DPoint struct in implot3d.h already implements most of these vector operations, so you can use ImPlot3DPoint instead. This Vec3 code is not needed.

will take that into account

brenocq · 2025-07-17T04:20:52Z

.gitignore

+
+## clangd
+compile_flags.txt


Just wondering, what did you use the compile_flags.txt for? 👀

this is for my neovim highlighting.

so it doesn't get added to the files.

Nvim best editor, yes or yes?

brenocq · 2025-07-17T04:31:29Z

implot3d_items.cpp

+		}
+	}
+
+	PlotQuad(label_id, quad_xs, quad_ys, quad_zs, (count - 1) * sides * 4);


Great to see that you are using PlotQuad for the rendering! It makes the code much easier to maintain.

thanks, that was the objective.

AhmedLilah added 3 commits June 8, 2025 23:50

Adding the PlotPrismLine function: plots an N-Sided Prism around a li…

5a7ae07

…ne to make it better looking in 3D and avoid plain-line intersection

Adding line weigth to the plot prism function

dc48e22

Adding the PlotPrismLine function: plots an N-Sided Prism around a li…

8da3a07

…ne to make it better looking in 3D and avoid plain-line intersection

AhmedLilah requested a review from brenocq as a code owner June 9, 2025 23:46

AhmedLilah added 2 commits June 10, 2025 02:50

removing the accidental inclusion of the "imgui.h" from implot3d_item…

9c8da45

…s.cpp

removing unsed macro from the PlotPrsimLine section in implot3d_items…

4c14c12

….cpp

brenocq requested changes Jul 17, 2025

View reviewed changes

brenocq changed the title ~~PlotPrsimLine~~ Feat: PlotPrismLine Jul 18, 2025

brenocq assigned AhmedLilah Jul 18, 2025

brenocq added type:feat New feature or request prio:medium Medium priority status:review The task is under review prio:high High priority and removed prio:medium Medium priority labels Jul 18, 2025

brenocq mentioned this pull request Jul 18, 2025

[Bug] Line Duplication Due To Line-Plane Intersection #126

Open


		## clangd
		compile_flags.txt

Feat: PlotPrismLine #129

Are you sure you want to change the base?

Feat: PlotPrismLine #129

Uh oh!

Conversation

AhmedLilah commented Jun 9, 2025 • edited by brenocq Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AhmedLilah commented Jun 19, 2025

Uh oh!

brenocq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AhmedLilah commented Jun 9, 2025 •

edited by brenocq

Loading

brenocq left a comment •

edited

Loading