search

otto-link / Hesiod

A desktop application for node-based procedural terrain generation.
https://hesioddoc.readthedocs.io/
GNU General Public License v3.0
111 stars 5 forks source link

Improve Brush node #120

Closed malpas closed 7 months ago

malpas commented 7 months ago

Hi, Hesiod looks great. I thought the brush node felt a bit placeholder, so I've reworked it and fixed a couple UI bugs in the process.

Changes

Screenshot

Capture

otto-link commented 7 months ago

Hey, thanks a lot for the feedback and for your contributions! Looks really nice, I'll handle your PR pretty quickly

malpas commented 7 months ago

Thanks for the feedback, I've made the changes. Looks like the error was for std::span, which compiled on MSVC without needing \<span>.

Those definitions aren't for bulletproofing the constructor. I've noticed there's a deeper issue that ViewNode doesn't have a virtual destructor, so the drawing texture may leak when ViewBrush destructs polymorphically.

Defining the virtual destructor for ViewNode will remove its default move construction/assignment, so you'll need to define move and copy semantics for it (rule of 5), same with ViewBrush. The compiler warnings were from me wrongly defaulting ViewBrush's move constructor.

I agree some of this gets outside the scope of the brush node, unless you feel strongly about the occasional texture leak for now. It might be worth it for you to take some time to decide specifically which of your classes are unique vs shared, and how they're destroyed. Many of those decisions are the kind of top-down changes that I believe would be best for you to make yourself. If nodes become clonable in the future, you might also decide later to add a virtual clone() function.

otto-link commented 7 months ago

I'm going to merge your PR as soon as I can. The resulting user experience feels really nice, especially comparing to the first mockup 😅 I'm about to integrate a new version of the "noise Node" (like fractal) with the ability to feed the fractal layering with an input heightmap (so that we'll be able to "draw" the fractal noise).

Let me know if there are some specific topics you'd like to work on!

For the Brush node, there are some new features that could be added:

For the virtual destructor topic, first thanks for spotting the issue. Having an external perspective is really valuable. I'm going to create a specific issue for that with some details on the overall class structures. I'll be glad to have your input on the pros/cons and what could be done to improve it.

malpas commented 7 months ago

I appreciate the kind words and will keep an eye out for anything that catches my interest. Right click subtraction should already work.

When it comes to user-facing terrain software, most people just want features to fit together nicely and feel satisfying to create realistic landscapes. We want a mountain here, river there, canyon, etc. It's a very tricky balance between power and UX. Looking forward to seeing what you create in the future.