Details

Reviewers

Campbell Barton (campbellbarton)
Howard Trickey (howardt)
Brecht Van Lommel (brecht)

Commits

rBb0786d069cee: Ocean modifier: fix changes to resolution complete changing the shape

Summary

A fundamental problem in the ocean modifier is that changing the resolution, for example, causes a complete change in the spectrum being generated. This is not desirable - changing the resolution should only add/remove detail from the underlying surface.

Amaan Akram's GPLv3 aaOcean library addresses this by ensuring the RNG seed is linked to the surface sample point. This diff brings this enhancement to blender's ocean modifier.

The patch consists of a UID method (taken from aaOcean directly), which takes the sample coordinates to generate the UID. This UID is then used with the user-specified seed to initialize the RNG.

Before:

After:

Diff Detail

Repository: rB Blender

Event Timeline

Phil Stopford (philstopford) created this revision.Feb 17 2020, 5:33 PM

Jonathan Lampel (jonathanl) awarded a token.Feb 17 2020, 7:41 PM

Phil Stopford (philstopford) added reviewers: Campbell Barton (campbellbarton), Howard Trickey (howardt).Feb 17 2020, 8:12 PM

Hans Goudey (HooglyBoogly) awarded a token.Feb 17 2020, 9:06 PM

The style guide recommends different comment formatting than what you're using. It might not be consistent in ocean.c, but it's worth getting new code to be consistent.

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

source/blender/blenkernel/intern/ocean.c
829	There's a macro for this, `RAD2DEGF`.

Aaron Carlisle (Blendify) awarded a token.Feb 17 2020, 9:43 PM

Used macro, changed comments to fit style guide.

noki paike (amonpaike) awarded a token.Feb 18 2020, 5:43 AM

Aaron Carlisle (Blendify) edited the summary of this revision. (Show Details)Feb 19 2020, 4:24 AM

Fran Marz (franMarz) awarded a token.Feb 19 2020, 5:11 AM

Generally seems fine, minor issues noted inline.

source/blender/blenkernel/intern/ocean.c
817	This comment should explain what needs to be improved, the down-sides with this implementation. Since the function is fairly small, any reason not to improve it before committing?
820	Unless you can show this makes a measurable difference, no need to use `register`, let the compiler handle this.
820	Convention for naming in Blender is to use snake_case: `coord_sq`.
822	Style use `uint`
829	Typically we use radians, not degrees, you can use `DEG2RAD(360)` for readability.
835–840	Style always use braces.

Brecht Van Lommel (brecht) requested changes to this revision.Feb 19 2020, 9:44 AM

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).

Brecht Van Lommel (brecht) added inline comments.

source/blender/blenkernel/intern/ocean.c
817	Have not tested, but perhaps this whole function could just be: return BLI_hash_int_2d((int)xCoord, (int)zCoord); Or something like this if the coordinates are not big enough before rounding: return BLI_hash_int_2d((int)(xCoord * 360.0f), (int)(zCoord * 360.0f));
820	Just never use `register`, it's pointless.
827–832	This test does nothing, as `acos` returns values in the range [0, pi]. The correct way to convert to polar coordinates should be like this (which also avoids division by zero and NaNs that the above code can have): float length = sqrtf(zCoord * zCoord + xCoord * xCoord); float angle = atan2f(zCoord, xCoord); if (angle < 0.0) { angle += 2.0f * (float)M_PI; } Not really relevant if `BLI_hash_int_2d` works though.
1024	This leaks memory, the is RNG is allocated many times but only freed once. Rather than allocating a RNG multiple times, leave `BLI_rng_new` once at the start, then call `BLI_rng_seed(rng, new_seed)` here.

This revision now requires changes to proceed.Feb 19 2020, 9:44 AM

Using blender's hash function with parameter scaling. Changed RNG approach to change seed of existing RNG rather than initialize a new RNG each time.

Changed to use blender's hash function (I went looking, but somehow missed it originally). RNG approach changed per guidance.

source/blender/blenkernel/intern/ocean.c
827–832	Discarding the generateUID in favor of the blender hash call.

Brecht Van Lommel (brecht) accepted this revision.Feb 19 2020, 2:24 PM

This revision is now accepted and ready to land.Feb 19 2020, 2:24 PM

Way awarded a token.Feb 19 2020, 8:24 PM

xrg (xrg) awarded a token.Feb 19 2020, 9:04 PM

Kenn Nyström (Frozen_Death_Knight) awarded a token.Feb 19 2020, 10:38 PM

Ray Molenkamp (LazyDodo) added a subscriber: Ray Molenkamp (LazyDodo).Feb 19 2020, 10:44 PM

Ray Molenkamp (LazyDodo) added inline comments.

source/blender/blenkernel/intern/ocean.c
990	nit picking here, double space after = should be a single space.

when you run 'make format' in the source folder all these nitpicky code style issues should resolve them selves

Addressing nitpick for spacing, using make format to check

Campbell Barton (campbellbarton) added inline comments.Feb 20 2020, 1:09 AM

source/blender/blenkernel/intern/ocean.c
820	Also thought `register` wasn't needed, giving author benefit of doubt since reading up on it suggests it's _possible_ there are some benefits, although references to this are about older compilers. In this case we should set a good example & remove register from Blender's code.