Page MenuHome

Adding new spectra to the ocean modifier
ClosedPublic

Authored by Phil Stopford (philstopford) on Feb 19 2020, 4:10 PM.
Tags
None
Tokens
"Love" token, awarded by jonathanl."Pterodactyl" token, awarded by Way."Love" token, awarded by eversimo."Love" token, awarded by dulrich."Love" token, awarded by franMarz.

Details

Summary

This work extends the ocean modifier to add new spectra (Pierson-Moskowitz, Jonswap, TMA). These models are very different to the Phillips spectrum. They are intended for more established, large area, oceans and/or shallow water situations.

The algorithms come from the Apache 2 licensed (GPLv3 compatible) EncinoWaves project, modified to work within the blender C file.

Diff Detail

Repository
rB Blender

Event Timeline

Way awarded a token.Sun, Mar 1, 1:28 PM
Brecht Van Lommel (brecht) requested changes to this revision.Sun, Mar 1, 1:57 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/ocean.c
216

Please move the Apache2 code in a separate ocean_spectrum.c file with full Apache license header for clarity (same as in e.g. intern/cycles code).

source/blender/makesdna/DNA_modifier_types.h
1278

Follow comment style guidelines and use /* */

source/blender/makesrna/intern/rna_modifier.c
5171

Remove MOD_OCEAN_SPECTRUM_ prefix from the identifiers, it's redundant.

5174

Remove Spectrum from the label, it's redundant.

5184

JONSWAP Spectrum -> Jonswap

5340

Remove comment, will not be an experiment once committed.

5350

Remove experimental

5353

Don't add tma in the proper identifier. Also this should get a better name, "prefetch" has a specific meaning in computing and it's not this.

I can't understand the meaning from the description.

5356

TMA/Jonswap -> TMA or Jonswap

5362

peak_sharpen_tma -> sharpen_peak
Peak sharpen -> Sharpen Peak

And the description is not a correct sentence.

source/blender/modifiers/intern/MOD_ocean.c
430

Please explain this change.

This revision now requires changes to proceed.Sun, Mar 1, 1:57 PM

Ideally the spectra should get descriptive names about what kind of waves they are intended for, with the author / technical name mentioned only in the description.

Moved apache licensed code to ocean_spectrum.c
Changed names to suggested values.
Hopefully fixed comments to use required convention.
Added some more comments to better explain 'fetch' for the JONSWAP model.
Made UI spectrum show guidance about the spectrum itself.
Removed obsolete 'surface_tension' float.

Phil Stopford (philstopford) marked 11 inline comments as done.Sun, Mar 1, 9:47 PM
Phil Stopford (philstopford) added inline comments.
source/blender/blenkernel/intern/ocean.c
216

This seems to lead me to a land of pain where I need fftw3.h in more projects than is currently the case because I need to move the Ocean struct into the header file from the C file, to make it available in the separate ocean_spectrum.c file.

This may not be desirable, but if it is, I'm not sure how to adjust the CMake configuration to ensure the appropriate header path is available for each affected project. Is there guidance somewhere?

source/blender/makesdna/DNA_modifier_types.h
1278

I keep making that mistake for the comments. I'll try and do better.

I actually want to drop surface_tension below, but this seems to break the build. I'm assuming that I need to adjust the padding

source/blender/makesrna/intern/rna_modifier.c
5184

Uppercase is convention here - it's an acronym : Joint North Sea Wave Observation Project

5350

I want to drop this - it's an abandoned experimental model. I missed it in the original submission

5353

An alternative would be fetch. Would that work for you? This parameter comes from the distance to a short in the lee of a wind:

'distance from a lee shore, called the fetch, or the distance over which the wind blows with constant velocity.'

source/blender/modifiers/intern/MOD_ocean.c
430

I was bothered that the call to the foam actually squared the calculated value before returning it; it made things confusing when reviewing the code. I felt an explicit square of the return value offered more clarity. The non-Phillips spectra don't really offer provide 'intense' foam, so I was opening up modification of the value here in case a spectra-dependent tweak might be desirable later.

This change is not mandatory and changes nothing functionally at this time.

Campbell Barton (campbellbarton) requested changes to this revision.Mon, Mar 2, 10:18 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/BKE_ocean.h
28

This should go into an internal header since it's not a part of BKE_ocean.h API.

We have pbvh_intern.h, data_transfer_intern.h for eg, this can be named ocean_intern.h accordingly.

128–131

Public functions should use BLI_ocean_spectrum_ prefix unless they are moved into ocean_intern.h, even in this case ocean_spectrum_ prefix would help identify their use.

source/blender/blenkernel/intern/ocean_spectrum.c
2

The full license isn't necessary, an abbreviated header is sufficient, cycles for example uses:

/*
 * Copyright 2011-2013 Blender Foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
This revision now requires changes to proceed.Mon, Mar 2, 10:18 AM
Phil Stopford (philstopford) marked 6 inline comments as done.Mon, Mar 2, 3:21 PM
Phil Stopford (philstopford) added inline comments.
source/blender/blenkernel/BKE_ocean.h
28

To clarify, you want the struct relocated to a new file?

Adjusted names of functions per feedback.
Abbreviated Apache license.
Moved struct to a dedicated header file.

Phil Stopford (philstopford) marked 3 inline comments as done.Mon, Mar 2, 7:13 PM

Is there anything pending that I overlooked in the requested changes?

Campbell Barton (campbellbarton) requested changes to this revision.Thu, Mar 5, 12:11 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/properties_data_modifier.py
723

Use md.spectrum in {'TEXELMARSENARSLOE', 'JONSWAP'}

source/blender/blenkernel/BKE_ocean.h
131

This is declared static in the source, declaration should be moved to top of source file.

source/blender/blenkernel/intern/ocean_intern.h
2

File misses license, doxy \file & header guard.

source/blender/blenkernel/intern/ocean_spectrum.c
3
source/blender/makesdna/DNA_modifier_types.h
1258

End sentences with full-stop, also for other comments in this patch.

1304

No typedef needed here, plain enum is fine.

source/blender/makesrna/intern/rna_modifier.c
5169

Use underscore in enum identifiers to separate words.

PIERSONMOSKOWITZ -> PIERSON_MOSKOWITZ, TEXELMARSENARSLOE -> TEXEL_MARSEN_ARSLOE ... etc.

This revision now requires changes to proceed.Thu, Mar 5, 12:11 AM

Enum typedef removed
Changed enum names to separate whole words with '_'
Moved static up in source file, removed from header.
Added license information to new header file.
Added guard to new header file.
Added doc block to header file.
Added full-stops to new comments per feedback.

Phil Stopford (philstopford) marked 7 inline comments as done.Thu, Mar 5, 1:22 AM
Phil Stopford (philstopford) added inline comments.
source/blender/makesdna/DNA_modifier_types.h
1258

Most existing comments I found seemed to omit full-stop, do you want those updated as well?

Campbell Barton (campbellbarton) requested changes to this revision.Thu, Mar 5, 5:34 AM

Looks good, only minor change requested.

source/blender/makesdna/DNA_modifier_types.h
1258

No, just for new code. Updating existing comments can be done as separate cleanup.

source/blender/makesrna/intern/rna_modifier.c
5174–5196

Keep the name short and the description need not repeat it:

eg:

"JONSWAP - Pierson-Moskowitz with peak sharpening",
"JONSWAP - Pierson-Moskowitz with peak sharpening"},

Could be something like this:

"Pierson-Moskowitz",
"A version of jonswap that includes with peak sharpening"},
This revision now requires changes to proceed.Thu, Mar 5, 5:34 AM
Phil Stopford (philstopford) marked an inline comment as done.Thu, Mar 5, 2:19 PM
Phil Stopford (philstopford) added inline comments.
source/blender/makesrna/intern/rna_modifier.c
5174–5196

I thought about that, but the immediacy of the information in the menu (avoiding the need to wait for the tooltip guidance to show up in order to read the description) had me taking the other approach. It seemed more user friendly. I'll defer to the expert guidance, but just wanted to check whether this was required.

Brecht also requested descriptive names in an earlier review :

'Ideally the spectra should get descriptive names about what kind of waves they are intended for, with the author / technical name mentioned only in the description.'

So, at the moment, I'm not certain this request is the right way to go.

Phil Stopford (philstopford) requested review of this revision.Thu, Mar 5, 7:20 PM
Phil Stopford (philstopford) marked an inline comment as done.

Request review again based on the comment regarding names/descriptions. I tend to think the existing approach is more user friendly compared to tooltips.

source/blender/makesrna/intern/rna_modifier.c
5174–5196

What I meant was names like this (not sure if they make sense):

  • Turbulent Sea
  • Established Sea
  • Established Sea (Sharp Peaks)
  • Shallow Water

And then leave all those technical names for the description only.

Changed names and descriptions per feedback.
Moved common JONSWAP code (shared between the JONSWAP and TMA choices) into a static float function 'jonswap'. The difference between the two is thus more apparent in code.
Simplified the calculation of the damping across all of the models.

These changes eliminate boilerplate.

Phil Stopford (philstopford) marked an inline comment as done.Thu, Mar 5, 9:26 PM

Could this land for 2.83 or will it come later?

Thanks, committed rB6ce709dceb8db65ec6baae21100a7ce93829b1f6 with the following changes.

  • Move the spectrum setting just above "Waves" in the UI, since it was showing above "Time" & "Resolution" which are fundamental settings for this modifier.
  • Only show spectrum settings when used as there are already many buttons in the UI here.
  • Added versioning to set the fetch_jonswap value to 120 since the result is a blank mesh when loading existing files where it's zero.
  • Adjusted descriptions for enum items (include technical terms last in parenthesis).
  • Removed foam *= foam; from MOD_ocean.c (can be applied separately).
  • Use const arguments where possible.
  • Fix build error when the modifier was disabled from CMake.
  • Applied clang-format
This revision is now accepted and ready to land.Thu, Mar 12, 5:58 AM