Page MenuHome
Differential D8336

Adding display resolution to ocean modifier
ClosedPublic
Actions

Authored by Phil Stopford (philstopford) on Jul 17 2020, 9:01 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Brecht Van Lommel (brecht)
Commits
rBa44299ccd117: Ocean Modifier: Add viewport resolution
Summary

Following work done in 2.83, the resolution control is now a real level-of-detail parameter. It is now useful to be able to set the resolution for display independently from render. This is true for both mesh generation and mesh deformation modes

For compatibility with old scenes, resolution is retained and is the render resolution. Old scenes loaded in have the value of resolution also applied to viewport_resolution. This allows newer scenes to be used in older versions without trouble

Reference requests :

https://blender.community/c/rightclickselect/JKbbbc/
https://blender.community/c/rightclickselect/jvbbbc/

Diff Detail

Repository
rB Blender

Event Timeline

Phil Stopford (philstopford) requested review of this revision.Jul 17 2020, 9:01 PM
Phil Stopford (philstopford) created this revision.
Phil Stopford (philstopford) edited the summary of this revision. (Show Details)Jul 17 2020, 9:03 PM
Hans Goudey (HooglyBoogly) added a comment.Jul 17 2020, 9:12 PM

This is a nice improvement. I expect many people adjust this before rendering. Especially now that changing the resolution doesn't change the shape.

The terms we usually use are "Viewport" and "Render" resolution, I would be consistent here.

For getting old files to work, check in the versioning_290.c. I've already done some versioning there for the ocean modifier there for 2.90.

Something like this should work:

if (!DNA_struct_elem_find(fd->filesdna, "OceanModifierData", "int", "viewport_resolution")) {
  for (Object *object = bmain->objects.first; object != NULL; object = object->id.next) {
    LISTBASE_FOREACH (ModifierData *, md, &object->modifiers) {
      if (md->type == eModifierType_Ocean) {
        OceanModifierData *omd = (OceanModifierData *)md;
        omd->viewport_resolution *= omd->resolution;
      }
    }
  }
}
source/blender/makesdna/DNA_modifier_types.h
1269

I would suggest just having two values, with the names I suggested above. Then reuse this value so we're not storing redundant information in DNA.

source/blender/modifiers/intern/MOD_ocean.c
588–589

Put these two in an aligned subcolumn like in other modifiers with both viewport and render resolution.

Hans Goudey (HooglyBoogly) requested changes to this revision.Jul 17 2020, 9:13 PM
This revision now requires changes to proceed.Jul 17 2020, 9:13 PM
Phil Stopford (philstopford) added a comment.EditedJul 17 2020, 11:29 PM
In D8336#202737, @Hans Goudey (HooglyBoogly) wrote:

The terms we usually use are "Viewport" and "Render" resolution, I would be consistent here.

For getting old files to work, check in the versioning_290.c. I've already done some versioning there for the ocean modifier there for 2.90.

Something like this should work:

if (!DNA_struct_elem_find(fd->filesdna, "OceanModifierData", "int", "viewport_resolution")) {
  for (Object *object = bmain->objects.first; object != NULL; object = object->id.next) {
    LISTBASE_FOREACH (ModifierData *, md, &object->modifiers) {
      if (md->type == eModifierType_Ocean) {
        OceanModifierData *omd = (OceanModifierData *)md;
        omd->viewport_resolution *= omd->resolution;
      }
    }
  }
}

OK, I think I might be misunderstanding, so wanted to check beforehand:

Legacy files have resolution defined. I was keeping that name in the newer code as I could populate it easily and not have to worry about people taking files from 2.90 back to 2.83 and losing their ocean settings. I added display_resolution (and can change that name). I used your versioning code (there's a small typo there - it should be a direct assignment, not a multiplier) and this now sets the display_resolution value correctly.

What I'm unclear on is the removal of ocean_resolution (if I understand you correctly). That is used internally to set the actual resolution for the modifier. I need it as part of the modifier data because the context is not available to me in BKE_ocean_init_from_modifier, so I use ocean_resolution to pass the value I really want to use. If there's a better approach, I'm OK with that - I just couldn't see it.

Further, where I run into trouble is that, on load of the blender file, I end up dying in ocean_compute_jacobian because the ocean_resolution is zero. The attempt to set it (in doOcean) seems to be completely by-passed and I've no clear idea how or why this should be.

 	blender.exe!ocean_compute_jacobian_jxz(TaskPool * pool, void * UNUSED_taskdata) Line 613	C
 	blender.exe!Task::()::__l2::<lambda>() Line 118	C++
 	blender.exe!tbb::interface7::internal::delegated_function<void <lambda>(void) const ,void>::operator()() Line 94	C++
 	[External Code]	
 	blender.exe!tbb::interface7::internal::isolate_impl<void,void <lambda>(void) const>(const Task::()::__l2::void <lambda>(void) & f) Line 161	C++
 	blender.exe!tbb::interface7::this_task_arena::isolate<void <lambda>(void)>(const Task::()::__l2::void <lambda>(void) & f) Line 396	C++
 	blender.exe!Task::operator()() Line 122	C++
 	blender.exe!tbb::internal::function_task<Task>::execute() Line 1049	C++
 	[External Code]	
 	blender.exe!tbb::task::wait_for_all() Line 810	C++
 	blender.exe!tbb::internal::task_group_base::wait() Line 168	C++
 	blender.exe!tbb_task_pool_work_and_wait(TaskPool * pool) Line 253	C++
 	blender.exe!BLI_task_pool_work_and_wait(TaskPool * pool) Line 500	C++
 	blender.exe!BKE_ocean_simulate(Ocean * o, float t, float scale, float chop_amount) Line 706	C
 	blender.exe!set_height_normalize_factor(Ocean * oc) Line 726	C
 	blender.exe!BKE_ocean_init(Ocean * o, int M, int N, float Lx, float Lz, float V, float l, float A, float w, float damp, float alignment, float depth, float time, int spectrum, float fetch_jonswap, float sharpen_peak_jonswap, short do_height_field, short do_chop, short do_normals, short do_jacobian, int seed) Line 1016	C
>	blender.exe!BKE_ocean_init_from_modifier(Ocean * ocean, const OceanModifierData * omd) Line 799	C
 	blender.exe!copyData(const ModifierData * md, ModifierData * target, const int flag) Line 174	C
 	blender.exe!BKE_modifier_copydata_ex(ModifierData * md, ModifierData * target, const int flag) Line 364	C
 	blender.exe!object_copy_data(Main * bmain, ID * id_dst, const ID * id_src, const int flag) Line 214	C
 	blender.exe!BKE_id_copy_ex(Main * bmain, const ID * id, ID * * r_newid, const int flag) Line 575	C
 	blender.exe!blender::deg::`anonymous namespace'::id_copy_inplace_no_main(const ID * id, ID * newid) Line 291	C++
 	blender.exe!blender::deg::deg_expand_copy_on_write_datablock(const blender::deg::Depsgraph * depsgraph, const blender::deg::IDNode * id_node, blender::deg::DepsgraphNodeBuilder * node_builder, bool create_placeholders) Line 898	C++
 	blender.exe!blender::deg::deg_update_copy_on_write_datablock(const blender::deg::Depsgraph * depsgraph, const blender::deg::IDNode * id_node) Line 953	C++
 	blender.exe!blender::deg::deg_evaluate_copy_on_write(Depsgraph * graph, const blender::deg::IDNode * id_node) Line 1088	C++
 	[External Code]	
 	blender.exe!blender::deg::`anonymous namespace'::evaluate_node(const blender::deg::`anonymous-namespace'::DepsgraphEvalState * state, blender::deg::OperationNode * operation_node) Line 116	C++
 	blender.exe!blender::deg::`anonymous namespace'::deg_task_run_func(TaskPool * pool, void * taskdata) Line 128	C++
 	blender.exe!Task::()::__l2::<lambda>() Line 118	C++
 	blender.exe!tbb::interface7::internal::delegated_function<void <lambda>(void) const ,void>::operator()() Line 94	C++
 	[External Code]	
 	blender.exe!tbb::interface7::internal::isolate_impl<void,void <lambda>(void) const>(const Task::()::__l2::void <lambda>(void) & f) Line 161	C++
 	blender.exe!tbb::interface7::this_task_arena::isolate<void <lambda>(void)>(const Task::()::__l2::void <lambda>(void) & f) Line 396	C++
 	blender.exe!Task::operator()() Line 122	C++
 	blender.exe!tbb::internal::function_task<Task>::execute() Line 1049	C++
 	[External Code]

In this call chain, I don't ever seem to have the context available to me in order to set ocean_resolution, so wonder if a fallback is required to satisfy a non-zero condition

Updated diff attached with the versioning code. I'll rename things later, happily. Just wanted to limit the churn until this issue is worked out.

Phil Stopford (philstopford) updated this revision to Diff 26940.Jul 17 2020, 11:30 PM

Versioning

Phil Stopford (philstopford) added a comment.Jul 17 2020, 11:46 PM

I can of course abuse the versioning system to set ocean_resolution on-load; I'm just not clear yet on the code flow that means the doOcean is not called to set this as I would have expected.

Phil Stopford (philstopford) added a comment.Jul 18 2020, 12:05 AM

An ugly workaround would be (in BKE_ocean_init_from_modifier)

int ocean_resolution = omd->ocean_resolution;
if (ocean_resolution < 1) {
  ocean_resolution = omd->resolution;
}

BKE_ocean_init(ocean,
               ocean_resolution * ocean_resolution,
               ocean_resolution * ocean_resolution,
               omd->spatial_size,
               omd->spatial_size,
               omd->wind_velocity,
               omd->smallest_wave,
               1.0,
               omd->wave_direction,
               omd->damp,
               omd->wave_alignment,
               omd->depth,
               omd->time,
               omd->spectrum,
               omd->fetch_jonswap,
               omd->sharpen_peak_jonswap,
               do_heightfield,
               do_chop,
               do_normals,
               do_jacobian,
               omd->seed);
Hans Goudey (HooglyBoogly) added a comment.Jul 18 2020, 2:45 AM

If there are bad values getting through versioning then the versioning isn't working correctly. The stuff you're doing in generate_ocean_geometry shouldn't be necessary.
If you're adding two variables to the ocean modifier both of them should have their values set with versioning.

What I'm unclear on is the removal of ocean_resolution (if I understand you correctly). That is used internally to set the actual resolution for the modifier. I need it as part of the modifier data because the context is not available to me in BKE_ocean_init_from_modifier, so I use ocean_resolution to pass the value I really want to use. If there's a better approach, I'm OK with that - I just couldn't see it.

Right, there really should be a better solution. We shouldn't be storing three values and only using two of them. I think the approach is to add another argument to BKE_ocean_init_from_modifier saying whether to use the render or viewport resolution.

Phil Stopford (philstopford) updated this revision to Diff 26946.Jul 18 2020, 5:00 AM

Finding a way to handle older scenes. Still WIP, pending renames (at the very least)

Phil Stopford (philstopford) added a comment.Jul 18 2020, 5:00 AM

@Hans Goudey (HooglyBoogly) : does this look cleaner to you?

Hans Goudey (HooglyBoogly) added a comment.Jul 18 2020, 3:07 PM

Yes, looks good!

source/blender/makesdna/DNA_modifier_types.h
1289

makesdna always tells you to add more, but I think a padding of 8 shouldn't be necessary and you could just remove the padding completely.

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

Maybe 3 is a bit low? Not sure though

Phil Stopford (philstopford) updated this revision to Diff 26952.Jul 18 2020, 3:49 PM

Renaming per earlier feedback, remove useless padding

Phil Stopford (philstopford) added a comment.Jul 18 2020, 3:50 PM

Here's the rename and grouping work for the UI. Padding also removed. Hopefully this ticks all the boxes.

Phil Stopford (philstopford) marked 4 inline comments as done.Jul 18 2020, 4:08 PM
Phil Stopford (philstopford) retitled this revision from WIP: Adding display resolution to ocean modifier to Adding display resolution to ocean modifier.Jul 18 2020, 4:20 PM
Phil Stopford (philstopford) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) added a comment.Jul 18 2020, 7:11 PM

I'm getting a crash when I open a file I saved without this patch, I wonder if you can reproduce that. I don't have a debug build around right now though.

source/blender/editors/object/object_modifier.c
2812

It probably does make sense to use the render resolution for baking the ocean, but it should be mentioned in the tooltip for the render resolution property. I suggest:
Resolution of the generated surface used for rendering and baking
and
Resolution of the generated surface used in the viewport.

source/blender/modifiers/intern/MOD_ocean.c
568–571

Above just a few lines you'll see the standard for how this is done. If these aren't in a sub column then there will be a larger space after these properties.

Also, we generally avoid repeating the resolution word in this situation.

Phil Stopford (philstopford) added a comment.Jul 18 2020, 7:25 PM

No crash here with a file saved from blender 2.83.

Diff incoming with the requested changes.

ocean283.blend97 KBDownload

Phil Stopford (philstopford) updated this revision to Diff 26956.Jul 18 2020, 7:25 PM

Tooltip and label updates

Phil Stopford (philstopford) marked 2 inline comments as done.Jul 18 2020, 7:26 PM
Phil Stopford (philstopford) edited the summary of this revision. (Show Details)Jul 19 2020, 6:21 PM
Phil Stopford (philstopford) added a comment.Jul 20 2020, 6:06 PM

@Hans Goudey (HooglyBoogly) : anything left for me to fix up here?

Hans Goudey (HooglyBoogly) requested changes to this revision.EditedJul 20 2020, 8:26 PM

Some smaller things here and more info on the crash I mentioned.

In a lite build without the ocean modifier I get this error:

/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:1610:6: error: conflicting types for ‘BKE_ocean_init_from_modifier’
 1610 | void BKE_ocean_init_from_modifier(struct Ocean *UNUSED(ocean),
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:44:
/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/BKE_ocean.h:74:6: note: previous declaration of ‘BKE_ocean_init_from_modifier’ was here
   74 | void BKE_ocean_init_from_modifier(struct Ocean *ocean, struct OceanModifierData const *omd, int resolution);
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

ASAN (https://wiki.blender.org/wiki/Tools/Debugging/ASAN_Address_Sanitizer) reports this crash I mentioned earlier. You should set this up, it will save you time in the end.
I don't have the time to look into this more now, but here's what I did.

  1. I saved the file on a recent build of 2.90 without this patch with the default startup file with an ocean modifier added (resolution set to 8)
  2. Then I compiled your patch and opened the file (which was saved as my startup file)
  3. Immediate crash with the following ASAN report:
/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:904:75: runtime error: -1040.5 is outside the range of representable values of type 'unsigned int'
/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:904:55: runtime error: -1040.5 is outside the range of representable values of type 'unsigned int'
=================================================================
==2456710==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000004038 at pc 0x000008d34f63 bp 0x7fffffff8590 sp 0x7fffffff8580
WRITE of size 4 at 0x602000004038 thread T0
    #0 0x8d34f62 in BKE_ocean_init /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:873
    #1 0x8d33777 in BKE_ocean_init_from_modifier /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:778
    #2 0x5a8ec4a in copyData /home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_ocean.c:171
    #3 0x36c5d3c in BKE_modifier_copydata_ex /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/modifier.c:361
    #4 0x37a15e5 in object_copy_data /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/object.c:213
    #5 0x35738cb in BKE_id_copy_ex /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/lib_id.c:573
    #6 0x921c1c0 in id_copy_inplace_no_main /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:291
    #7 0x922103f in blender::deg::deg_expand_copy_on_write_datablock(blender::deg::Depsgraph const*, blender::deg::IDNode const*, blender::deg::DepsgraphNodeBuilder*, bool) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:898
    #8 0x92218af in blender::deg::deg_update_copy_on_write_datablock(blender::deg::Depsgraph const*, blender::deg::IDNode const*) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:952
    #9 0x92228b3 in blender::deg::deg_evaluate_copy_on_write(Depsgraph*, blender::deg::IDNode const*) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:1087
    #10 0x916fd8a in void std::__invoke_impl<void, void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>(std::__invoke_other, void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*&&, blender::deg::IDNode*&) /usr/include/c++/10/bits/invoke.h:60
    #11 0x916b436 in std::__invoke_result<void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>::type std::__invoke<void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>(void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*&&, blender::deg::IDNode*&) /usr/include/c++/10/bits/invoke.h:95
    #12 0x916303f in void std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>::__call<void, Depsgraph*&&, 0ul, 1ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul>) /usr/include/c++/10/functional:416
    #13 0x915c416 in void std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>::operator()<Depsgraph*, void>(Depsgraph*&&) /usr/include/c++/10/functional:499
    #14 0x91529c1 in void std::__invoke_impl<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>(std::__invoke_other, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*&&) /usr/include/c++/10/bits/invoke.h:60
    #15 0x9147c9b in std::enable_if<is_invocable_r_v<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>, void>::type std::__invoke_r<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>(std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*&&) /usr/include/c++/10/bits/invoke.h:110
    #16 0x913fcd7 in std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&) /usr/include/c++/10/bits/std_function.h:291
    #17 0x9218dc0 in std::function<void (Depsgraph*)>::operator()(Depsgraph*) const /usr/include/c++/10/bits/std_function.h:622
    #18 0x921404e in evaluate_node /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:114
    #19 0x9214098 in deg_task_run_func /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:125
    #20 0x1eb1a768 in Task::operator()() const::{lambda()#1}::operator()() const /home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:118
    #21 0x1eb1ba04 in tbb::interface7::internal::delegated_function<Task::operator()() const::{lambda()#1} const, void>::operator()() const /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:93
    #22 0x4717cd4 in tbb::interface7::internal::isolate_within_arena(tbb::interface7::internal::delegate_base&, long) (/home/hans/Documents/Blender-Git/build_linux_debug/bin/blender+0x4717cd4)
    #23 0x1eb1ad4f in void tbb::interface7::internal::isolate_impl<void, Task::operator()() const::{lambda()#1} const>(Task::operator()() const::{lambda()#1} const&) /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:160
    #24 0x1eb1aa3f in tbb::interface7::internal::return_type_or_void<Task::operator()() const::{lambda()#1}>::type tbb::interface7::this_task_arena::isolate<Task::operator()() const::{lambda()#1}>(tbb::interface7::internal::return_type_or_void const&) /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:395
    #25 0x1eb1a820 in Task::operator()() const /home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:118
    #26 0x1eb1b8ab in tbb::internal::function_task<Task>::execute() /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task.h:1048
    #27 0x47253d4 in tbb::internal::custom_scheduler<tbb::internal::IntelSchedulerTraits>::process_bypass_loop(tbb::internal::context_guard_helper<false>&, tbb::task*, long) (/home/hans/Documents/Blender-Git/build_linux_debug/bin/blender+0x47253d4)
    #28 0x472568a in tbb::internal::custom_scheduler<tbb::internal::IntelSchedulerTraits>::local_wait_for_all(tbb::task&, tbb::task*) (/home/hans/Documents/Blender-Git/build_linux_debug/bin/blender+0x472568a)
    #29 0xb81509b in tbb::task::wait_for_all() /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task.h:809
    #30 0xb8311a7 in tbb::internal::task_group_base::wait() /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_group.h:168
    #31 0x1eb183ee in tbb_task_pool_work_and_wait /home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:250
    #32 0x1eb19909 in BLI_task_pool_work_and_wait /home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:499
    #33 0x9216982 in blender::deg::deg_evaluate_on_refresh(blender::deg::Depsgraph*) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:392
    #34 0x90a9c7b in DEG_evaluate_on_refresh /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/depsgraph_eval.cc:63
    #35 0x39c8610 in scene_graph_update_tagged /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/scene.c:1501
    #36 0x39c871b in BKE_scene_graph_update_tagged /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/scene.c:1540
    #37 0x47401e5 in wm_event_do_depsgraph /home/hans/Documents/Blender-Git/blender/source/blender/windowmanager/intern/wm_event_system.c:359
    #38 0x4779dd8 in wm_file_read_post /home/hans/Documents/Blender-Git/blender/source/blender/windowmanager/intern/wm_files.c:630
    #39 0x477ca99 in wm_homefile_read /home/hans/Documents/Blender-Git/blender/source/blender/windowmanager/intern/wm_files.c:1144
    #40 0x479e328 in WM_init /home/hans/Documents/Blender-Git/blender/source/blender/windowmanager/intern/wm_init_exit.c:293
    #41 0x336e917 in main /home/hans/Documents/Blender-Git/blender/source/creator/creator.c:453
    #42 0x7ffff7055041 in __libc_start_main (/lib64/libc.so.6+0x27041)
    #43 0x336dead in _start (/home/hans/Documents/Blender-Git/build_linux_debug/bin/blender+0x336dead)

0x602000004038 is located 0 bytes to the right of 8-byte region [0x602000004030,0x602000004038)
allocated by thread T0 here:
    #0 0x7ffff7670667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667)
    #1 0x1eb581e8 in MEM_lockfree_mallocN /home/hans/Documents/Blender-Git/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:273
    #2 0x8d3499f in BKE_ocean_init /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:859
    #3 0x8d33777 in BKE_ocean_init_from_modifier /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:778
    #4 0x5a8ec4a in copyData /home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_ocean.c:171
    #5 0x36c5d3c in BKE_modifier_copydata_ex /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/modifier.c:361
    #6 0x37a15e5 in object_copy_data /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/object.c:213
    #7 0x35738cb in BKE_id_copy_ex /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/lib_id.c:573
    #8 0x921c1c0 in id_copy_inplace_no_main /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:291
    #9 0x922103f in blender::deg::deg_expand_copy_on_write_datablock(blender::deg::Depsgraph const*, blender::deg::IDNode const*, blender::deg::DepsgraphNodeBuilder*, bool) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:898
    #10 0x92218af in blender::deg::deg_update_copy_on_write_datablock(blender::deg::Depsgraph const*, blender::deg::IDNode const*) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:952
    #11 0x92228b3 in blender::deg::deg_evaluate_copy_on_write(Depsgraph*, blender::deg::IDNode const*) /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc:1087
    #12 0x916fd8a in void std::__invoke_impl<void, void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>(std::__invoke_other, void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*&&, blender::deg::IDNode*&) /usr/include/c++/10/bits/invoke.h:60
    #13 0x916b436 in std::__invoke_result<void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>::type std::__invoke<void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*, blender::deg::IDNode*&>(void (*&)(Depsgraph*, blender::deg::IDNode const*), Depsgraph*&&, blender::deg::IDNode*&) /usr/include/c++/10/bits/invoke.h:95
    #14 0x916303f in void std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>::__call<void, Depsgraph*&&, 0ul, 1ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul>) /usr/include/c++/10/functional:416
    #15 0x915c416 in void std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>::operator()<Depsgraph*, void>(Depsgraph*&&) /usr/include/c++/10/functional:499
    #16 0x91529c1 in void std::__invoke_impl<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>(std::__invoke_other, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*&&) /usr/include/c++/10/bits/invoke.h:60
    #17 0x9147c9b in std::enable_if<is_invocable_r_v<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>, void>::type std::__invoke_r<void, std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*>(std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)>&, Depsgraph*&&) /usr/include/c++/10/bits/invoke.h:110
    #18 0x913fcd7 in std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, blender::deg::IDNode*))(Depsgraph*, blender::deg::IDNode const*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&) /usr/include/c++/10/bits/std_function.h:291
    #19 0x9218dc0 in std::function<void (Depsgraph*)>::operator()(Depsgraph*) const /usr/include/c++/10/bits/std_function.h:622
    #20 0x921404e in evaluate_node /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:114
    #21 0x9214098 in deg_task_run_func /home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:125
    #22 0x1eb1a768 in Task::operator()() const::{lambda()#1}::operator()() const /home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:118
    #23 0x1eb1ba04 in tbb::interface7::internal::delegated_function<Task::operator()() const::{lambda()#1} const, void>::operator()() const /home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:93
    #24 0x4717cd4 in tbb::interface7::internal::isolate_within_arena(tbb::interface7::internal::delegate_base&, long) (/home/hans/Documents/Blender-Git/build_linux_debug/bin/blender+0x4717cd4)
    #25 0x7fffffffb3ef  ([stack]+0x1e3ef)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/ocean.c:873 in BKE_ocean_init
Shadow bytes around the buggy address:
  0x0c047fff87b0: fa fa 00 fa fa fa 00 fa fa fa fa fa fa fa 00 fa
  0x0c047fff87c0: fa fa 00 fa fa fa fa fa fa fa fd fd fa fa 00 fa
  0x0c047fff87d0: fa fa 00 00 fa fa fa fa fa fa 00 fa fa fa fa fa
  0x0c047fff87e0: fa fa 00 fa fa fa fa fa fa fa 00 fa fa fa fa fa
  0x0c047fff87f0: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
=>0x0c047fff8800: fa fa 00 fa fa fa 00[fa]fa fa 00 fa fa fa 00 fa
  0x0c047fff8810: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 00
  0x0c047fff8820: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c047fff8830: fa fa fd fa fa fa 00 00 fa fa 00 fa fa fa 00 00
  0x0c047fff8840: fa fa 00 00 fa fa 00 fa fa fa 00 00 fa fa 00 fa
  0x0c047fff8850: fa fa 00 fa fa fa 00 00 fa fa 00 00 fa fa 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==2456710==ABORTING

Also, I just printed out the resolution in BKE_ocean_init_from_modifier, with both viewport and render set to 8, and it sometimes prints out 7.

Resolution: 7
Resolution: 8
Resolution: 7
Resolution: 8
source/blender/blenkernel/intern/ocean.c
767

Please use clang format, these lines are too long

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

source/blender/blenloader/intern/versioning_290.c
379–389

This should be in the section below: * Versioning code until next subversion bump goes here.

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

I believe this property should have an "update" function set.

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

By making this the subcolumn of layout instead of the column col, this is placed at the bottom of all the other properties.

569–570

The property names should still still be descriptive. Right now you have a property name called "Viewport" in the ocean modifier. No one will have a clue what that's supposed to mean.

Like right above, specify the labels with IFACE_.

This revision now requires changes to proceed.Jul 20 2020, 8:26 PM
Phil Stopford (philstopford) updated this revision to Diff 27004.Jul 20 2020, 9:41 PM

Moved versioning code into the correct place (possibly this caused the crash Hans reported).
Fixed up the without-ocean-modifier function to take the additional resolution parameter.
Fixing up the UI entries (hopefully, although I find this part of blender's code to be rather confusing)

Phil Stopford (philstopford) updated this revision to Diff 27005.Jul 20 2020, 9:49 PM
Phil Stopford (philstopford) marked an inline comment as done.

'make format'

Phil Stopford (philstopford) marked an inline comment as done.Jul 20 2020, 9:54 PM

I couldn't reproduce the crash. I wonder if it might have been arising from the misplaced versioning code. If it still happens with this patch, could you supply the blend file?

This update also fixes the 'no oceansim' approach. I meant to test that, but forgot. I did notice in the past that I could still compile blender with functions that differed from the declaration (i.e. in the C file, have resolution as a parameter, without any such parameter in the header). This would build fine and run, but the moment the ocean modifier was invoked, the application would consume all the process space (64 GB) very quickly and then crash. I'm not sure why the compiler doesn't fail in this situation.

source/blender/blenkernel/intern/ocean.c
767

Gah. Done.

source/blender/blenloader/intern/versioning_290.c
379–389

I was fooled by the comment 'keep this message at the bottom of the function. Makes sense now, but when just passing through the file to make a change, the comment could be misleading (at least, it stopped me from adding to the end of the block.

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

I think this already has it as RNA_def_property_update(prop, 0, "rna_OceanModifier_init_update");

unless I misunderstand you.

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

Better?

569–570

Hopefully this is now better. I don't really fully follow the blender UI code, so it's been stumbling around rather than a clear plan

Hans Goudey (HooglyBoogly) added a comment.Jul 20 2020, 10:38 PM

Okay, the crash is gone for me now.

@Brecht Van Lommel (brecht), if you wanted to look at this, I think it's ready other than the UI changes and property names I'll make when committing.

source/blender/blenloader/intern/versioning_290.c
379–389

Actually reading that again it is confusing!

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

Oh yes, oops, I was fooled by the diff highlighting.

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

Yes, thanks!

569–570

Haha not exactly what I meant, I guess I wasn't clear enough. It's easy enough for me adjust when committing though, easier than the back and forth, and then you can see what I mean.

Hans Goudey (HooglyBoogly) accepted this revision.Jul 20 2020, 10:39 PM
This revision is now accepted and ready to land.Jul 20 2020, 10:39 PM
Phil Stopford (philstopford) marked an inline comment as done.Jul 22 2020, 7:43 PM

Just curious - missed the cutoff for 2.90 or still a chance?

Hans Goudey (HooglyBoogly) added a comment.Jul 23 2020, 12:18 AM

Yeah, will unfortunately have to be 2.91 at this point.

Phil Stopford (philstopford) added a comment.Jul 27 2020, 6:10 PM

@Brecht Van Lommel (brecht) : when would this be merged for 2.91? I know 2.90 is the focus - this isn't intended to hassle, just curious.

Brecht Van Lommel (brecht) added a comment.Jul 30 2020, 5:50 PM

@Hans Goudey (HooglyBoogly) feel free to commit this if my minor comment is addressed.

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

Render -> Render Resolution. The RNA property name should be complete, the name in the UI code may be abbreviated if the context makes it clear.

You can also then remove the name override from the interface code.

Phil Stopford (philstopford) added a comment.Aug 3 2020, 5:59 AM

@Hans Goudey (HooglyBoogly) : would you be able to find time to commit this soon? :)

Hans Goudey (HooglyBoogly) added a comment.Aug 3 2020, 3:15 PM

If you make the update that Brecht requested I can commit this today.

Phil Stopford (philstopford) updated this revision to Diff 27362.Aug 3 2020, 3:27 PM

Hopefully this is what was asked for. Changed the property names and removed the overrides for the UI.

Closed by commit rBa44299ccd117: Ocean Modifier: Add viewport resolution (authored by Phil Stopford (philstopford), committed by Hans Goudey (HooglyBoogly)). · Explain WhyAug 3 2020, 4:08 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rBa44299ccd117: Ocean Modifier: Add viewport resolution.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
4 lines
intern/
14 lines
blenloader/
intern/
11 lines
editors/
object/
2 lines
makesdna/
6 lines
makesrna/
intern/
11 lines
modifiers/
intern/
33 lines

Diff 27004

View Options

source/blender/blenkernel/BKE_ocean.h

Loading...
View Options

source/blender/blenkernel/intern/ocean.c

Loading...
View Options

source/blender/blenloader/intern/versioning_290.c

Loading...
View Options

source/blender/editors/object/object_modifier.c

Loading...
View Options

source/blender/makesdna/DNA_modifier_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_modifier.c

Loading...
View Options

source/blender/modifiers/intern/MOD_ocean.c

Loading...