Skip to content

Builder patter for noise - #73

Open
Lamakaio wants to merge 3 commits into
ElliottjPierce:mainfrom
Lamakaio:main
Open

Lamakaio wants to merge 3 commits into
ElliottjPierce:mainfrom
Lamakaio:main

Conversation

@Lamakaio

Copy link
Copy Markdown

This implements a builder pattern for Noise, as mentioned in #72 .

The NoiseBuilder (and associated LayeredBuilder) both have an inner type to represent the current noise, and all methods consume the builder and return a new NoiseBuilder object with a different type.

The final noise produced with .get_noise and similar methods should be exactly the same as what would have been written by hand (meaning, fully typed).

I don't see how we could have a dyn-compatible builder trait, which would enable full reflection. I don't think full reflection without performance overhead is really possible ? But I might be wrong.

The documentation is very hasty right now, it needs to be rewritten.

@ElliottjPierce ElliottjPierce left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I know this is still a draft, and this is not a full review, but some quick thoughts:

  • The cleanup in the show_noise example really demonstrates the use to me. I definitely want to merge some form of this.
  • The tuple traits are very good.
  • You've mentioned this already, but expanding on the docs a bit would be good. Not only for the builders but also defining the structure of the types produced by the tuple traits.
  • I'd like to do layered noise building differently. In particular, octave and warp are mostly useless because they can't be used inside of a fractal layer. And fractal layers can't be nested. One possibility is a LayerBuilder to parallel NoiseBuilder. Maybe the interface is then fn NoiseBuilder::layered(R, W, impl FnOnce(LayerBuilder) -> LayerBuilder). And fn LayerBuilder::fractal(lacunarity, amount, impl FnOnce(LayerBuilder) -> LayerBuilder). Then we could add some common utilities on top like NoiseBuilder::fbm or something.
  • I'd also like to make the new take in an actual noise. I feel like new().chain(...) everywhere is not as nice as new(...). But that may just be preference honestly.

I'll do a full review when it's no longer a draft. And if you don't feel like re-working the LayeredBuilder in this pr, that's ok. I can work on that after merging this.

I'm very excited about this. I'm still not sure it would work, thinking about it more, but I want to experiment with this for reflection. There's potential here.

@Lamakaio

Lamakaio commented Jul 8, 2025

Copy link
Copy Markdown
Author

I think the last commit addresses these issues.

There are a few things I'm still unsure about :

  • The very simple methods such as scale, snorm and unorm : are they useful ? Or is it better to keep the API simple with mostly chains ?
    I feel like .scale(0.5) is a bit less of a mental load to read than .chain(Scaled(0.5)). But it adds a method for very little gain.
  • Should the methods use opinionated default (e.g. Persitence and Normed for layered noise, with options to change it in the builder) ? It make the "easy path" easier but also needs additional methods to keep all the features available. (Methods that I need to add if we decide this is the way, though it should be fast).

@ElliottjPierce

Copy link
Copy Markdown
Owner
  • The very simple methods such as scale, snorm and unorm : are they useful ? Or is it better to keep the API simple with mostly chains ? I feel like .scale(0.5) is a bit less of a mental load to read than .chain(Scaled(0.5)). But it adds a method for very little gain.

Let's keep things long and simple for now. Ex: .chain(Scaled(0.5)). I'm not opposed to exposing helper functions for common things, but that should be a future PR.

  • Should the methods use opinionated default (e.g. Persitence and Normed for layered noise, with options to change it in the builder) ? It make the "easy path" easier but also needs additional methods to keep all the features available. (Methods that I need to add if we decide this is the way, though it should be fast).

I'd rather not have opinionated defaults anywhere. For example, that could create situations where changing the default causes breaking changes. I'd much rather keep things explicit (and in the future provide helper functions to make it super low-friction.)

Thanks again for making this. I'll do more review when I get the chance.

@ElliottjPierce
ElliottjPierce marked this pull request as ready for review July 8, 2025 19:15
@ElliottjPierce
ElliottjPierce marked this pull request as draft July 8, 2025 19:15
@ElliottjPierce

Copy link
Copy Markdown
Owner

(Misclicked as ready for review. Oops. But maybe it is ready? Up to you.)

@Lamakaio

Lamakaio commented Jul 9, 2025

Copy link
Copy Markdown
Author

Alright, when I address these two points the PR should be ready for review.

Also clippy breaks on something that's not from my PR, not sure if I should fix it.

@ElliottjPierce

Copy link
Copy Markdown
Owner

Alright, when I address these two points the PR should be ready for review.

Awesome!

Also clippy breaks on something that's not from my PR, not sure if I should fix it.

Yup my bad. Must be a new clippy lint. You're welcome to fix it real quick if you like. Otherwise, I can fix it later and you can update the branch. It doesn't matter to me.

@Lamakaio
Lamakaio marked this pull request as ready for review July 9, 2025 14:59
@Lamakaio

Copy link
Copy Markdown
Author

I marked ready for review but kind forgot to comment to send a notification oops. Anyway it's good on my side @ElliottjPierce .

Maybe it's best to merge this before bevy's rc start coming ?

@ElliottjPierce

Copy link
Copy Markdown
Owner

Yup! I'll probably review and merge this sometime this week. I might extend this a little, but I'm pretty happy with your design. Thanks again for building this!

@ElliottjPierce

Copy link
Copy Markdown
Owner

Sorry I didn't get to it as soon as I thought. I went on vacation.

Now that I'm back, I've prototyped a working version of dynamic noise through a builder pattern. I may have figured out a way to use a similar design to get full runtime-defined, reflect-able, builder-pattern noise that produces the same ASM is if it were defined statically! But that will take more time to pursue to its conclusion.

I also recently looked at another new noise crate that uses SIMD with a builder pattern. That crate even out-performs noiz by a lot depending on the platform and use-case.

All that to say, I'm not sure if this pr is the right direction anymore. It certainly could be, but I want to do more research and exploration first, and TBH, I'm not sure when I'll have the time to do that... I'm working on like 3 other bevy projects right now too. If/when I figure out what to do here, I'll let you know, but until then, I'm going to leave this open for me to come back to later. Regardless, thanks for prototyping this! It's a good reference for my testing and future implementations.

@Lamakaio

Lamakaio commented Aug 7, 2025

Copy link
Copy Markdown
Author

Ohh that sounds cool if it is possible ! And yeah sure that works, I've been caught up in other projects as well anyway 😅

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.

2 participants