[3.x] Added ring emitter for 3D particles #47801

QbieShay · 2021-04-11T16:11:06Z

This PR implements the ring emitter for 3.x particles.

It does not have a 4.0 counter part because it will be covered in this PR already: #42248

If this change is desired, I'll update the CPU particles as well.

Let me know!

hpvb · 2021-04-11T17:13:01Z

LGTM!

qarmin · 2021-04-11T17:13:25Z

Crash in CI is not related with this PR(I think), but uncovers strange crash with multithreading, GradientTexture and Particles/Spatial/Standard Material

Address sanitizer log

 ==8621==ERROR: AddressSanitizer: heap-use-after-free on address 0x61400000def0 at pc 0x00001182b752 bp 0x7fdea600c200 sp 0x7fdea600c1f0
READ of size 4 at 0x61400000def0 thread T11
    #0 0x1182b751 in std::__atomic_base<unsigned int>::load(std::memory_order) const /usr/include/c++/9/bits/atomic_base.h:419
    #1 0x1182b751 in SafeNumeric<unsigned int>::get() const core/safe_refcount.h:70
    #2 0x1182b751 in Reference::unreference() core/reference.cpp:92
    #3 0x17caf80 in Ref<Reference>::unref() core/reference.h:277
    #4 0x1182921a in RefPtr::unref() core/ref_ptr.cpp:90
    #5 0x11978ea2 in Variant::clear() core/variant.cpp:1129
    #6 0x1714581 in Variant::~Variant() core/variant.h:444
    #7 0x1c3f45c in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element::~Element() core/map.h:50
    #8 0x1c3f554 in void memdelete_allocator<Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element, DefaultAllocator>(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/os/memory.h:128
    #9 0x1c1a548 in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:505
    #10 0x1c1a53c in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:504
    #11 0x1c1a53c in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:504
    #12 0x1c1a495 in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:503
    #13 0x1c1a53c in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:504
    #14 0x1c1a53c in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::_cleanup_tree(Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::Element*) core/map.h:504
    #15 0x1bfd24d in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::clear() core/map.h:660
    #16 0x1bfe4af in Map<StringName, Variant, Comparator<StringName>, DefaultAllocator>::~Map() core/map.h:681
    #17 0x7dba7ae in RasterizerStorageGLES3::Material::~Material() (/home/runner/work/godot/godot/bin/godot.x11.tools.64s+0x7dba7ae)
    #18 0x7dbaadf in void memdelete<RasterizerStorageGLES3::Material>(RasterizerStorageGLES3::Material*) (/home/runner/work/godot/godot/bin/godot.x11.tools.64s+0x7dbaadf)
    #19 0x7d610aa in RasterizerStorageGLES3::free(RID) drivers/gles3/rasterizer_storage_gles3.cpp:8152
    #20 0x105a0250 in VisualServerRaster::free(RID) servers/visual/visual_server_raster.cpp:72
    #21 0x10a1c031 in CommandQueueMT::Command1<VisualServer, void (VisualServer::*)(RID), RID>::call() core/command_queue_mt.h:301
    #22 0x10305869 in CommandQueueMT::flush_one(bool) core/command_queue_mt.h:440
    #23 0x1030629f in CommandQueueMT::wait_and_flush_one() core/command_queue_mt.h:473
    #24 0x10786b99 in VisualServerWrapMT::thread_loop() servers/visual/visual_server_wrap_mt.cpp:72
    #25 0x107860be in VisualServerWrapMT::_thread_callback(void*) servers/visual/visual_server_wrap_mt.cpp:57
    #26 0x11cbc3a7 in Thread::callback(Thread*, Thread::Settings const&, void (*)(void*), void*) core/os/thread.cpp:75
    #27 0x11cc1e09 in void std::__invoke_impl<void, void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(std::__invoke_other, void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/include/c++/9/bits/invoke.h:60
    #28 0x11cc1733 in std::__invoke_result<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>::type std::__invoke<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/include/c++/9/bits/invoke.h:95
    #29 0x11cc11d9 in void std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) /usr/include/c++/9/thread:244
    #30 0x11cc0e20 in std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::operator()() /usr/include/c++/9/thread:251
    #31 0x11cc0d58 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> > >::_M_run() /usr/include/c++/9/thread:195
    #32 0x12cec1c3 in execute_native_thread_routine (/home/runner/work/godot/godot/bin/godot.x11.tools.64s+0x12cec1c3)
    #33 0x7fdec58a1608 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
    #34 0x7fdec4d04292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)

0x61400000def0 is located 176 bytes inside of 448-byte region [0x61400000de40,0x61400000e000)
freed by thread T0 here:
    #0 0x7fdec5ddf7cf in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)
    #1 0x11ca5960 in Memory::free_static(void*, bool) core/os/memory.cpp:178
    #2 0x17da49e in void memdelete<Reference>(Reference*) core/os/memory.h:119
    #3 0x17cafe9 in Ref<Reference>::unref() core/reference.h:279
    #4 0x1182921a in RefPtr::unref() core/ref_ptr.cpp:90
    #5 0x11978ea2 in Variant::clear() core/variant.cpp:1129
    #6 0x1714581 in Variant::~Variant() core/variant.h:444
    #7 0x1715371 in CowData<Variant>::_unref(void*) core/cowdata.h:212
    #8 0x1714d60 in CowData<Variant>::~CowData() core/cowdata.h:388
    #9 0x17147a8 in Vector<Variant>::~Vector() core/vector.h:126
    #10 0x114be432 in ArrayPrivate::~ArrayPrivate() core/array.cpp:38
    #11 0x114be4b2 in void memdelete<ArrayPrivate>(ArrayPrivate*) core/os/memory.h:117
    #12 0x114b785f in Array::_unref() const core/array.cpp:68
    #13 0x114b74fd in Array::_ref(Array const&) const core/array.cpp:57
    #14 0x114b8015 in Array::operator=(Array const&) core/array.cpp:113
    #15 0x11999a7f in Variant::operator=(Variant const&) core/variant.cpp:2690
    #16 0x1d7e855 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:738
    #17 0x1bba565 in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #18 0x1175bd4b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #19 0x119e6047 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #20 0x1d8cccb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1086
    #21 0x1bbb2bf in GDScriptInstance::_ml_call_reversed(GDScript*, StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1239
    #22 0x1bbb575 in GDScriptInstance::call_multilevel_reversed(StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1246
    #23 0xc1ce5ce in Node::_notification(int) scene/main/node.cpp:149
    #24 0x1a65d39 in Node::_notificationv(int, bool) scene/main/node.h:46
    #25 0x1175c67a in Object::notification(int, bool) core/object.cpp:929
    #26 0xc1d0575 in Node::_propagate_ready() scene/main/node.cpp:196
    #27 0xc220584 in Node::_set_tree(SceneTree*) scene/main/node.cpp:2623
    #28 0xc1f193e in Node::_add_child_nocheck(Node*, StringName const&) scene/main/node.cpp:1149
    #29 0xc1f2fc0 in Node::add_child(Node*, bool) scene/main/node.cpp:1168

previously allocated by thread T0 here:
    #0 0x7fdec5ddfbc8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8)
    #1 0x11ca491d in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:82
    #2 0x11ca482e in operator new(unsigned long, char const*) core/os/memory.cpp:42
    #3 0xc1387a3 in Object* ClassDB::creator<GradientTexture>() (/home/runner/work/godot/godot/bin/godot.x11.tools.64s+0xc1387a3)
    #4 0x114dbddb in ClassDB::instance(StringName const&) core/class_db.cpp:559
    #5 0x122f5706 in _ClassDB::instance(StringName const&) const core/bind/core_bind.cpp:2900
    #6 0x23e38f4 in MethodBind1RC<Variant, StringName const&>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:1333
    #7 0x1175c1de in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:919
    #8 0x119e6047 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #9 0x1d8cc4a in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1083
    #10 0x1bba565 in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #11 0x1175bd4b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #12 0x119e6047 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #13 0x1d8cc4a in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1083
    #14 0x1bba565 in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #15 0x1175bd4b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #16 0x119e6047 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #17 0x1d8cc4a in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1083
    #18 0x1bba565 in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #19 0x1175bd4b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #20 0x119e6047 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #21 0x1d8cccb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1086
    #22 0x1bbb2bf in GDScriptInstance::_ml_call_reversed(GDScript*, StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1239
    #23 0x1bbb575 in GDScriptInstance::call_multilevel_reversed(StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1246
    #24 0xc1ce5ce in Node::_notification(int) scene/main/node.cpp:149
    #25 0x1a65d39 in Node::_notificationv(int, bool) scene/main/node.h:46
    #26 0x1175c67a in Object::notification(int, bool) core/object.cpp:929
    #27 0xc1d0575 in Node::_propagate_ready() scene/main/node.cpp:196
    #28 0xc220584 in Node::_set_tree(SceneTree*) scene/main/node.cpp:2623
    #29 0xc1f193e in Node::_add_child_nocheck(Node*, StringName const&) scene/main/node.cpp:1149

Thread T11 created by T0 here:
    #0 0x7fdec5d0c805 in pthread_create (/lib/x86_64-linux-gnu/libasan.so.5+0x3a805)
    #1 0x12cec488 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/home/runner/work/godot/godot/bin/godot.x11.tools.64s+0x12cec488)
    #2 0x11cbc896 in Thread::start(void (*)(void*), void*, Thread::Settings const&) core/os/thread.cpp:91
    #3 0x10788408 in VisualServerWrapMT::init() servers/visual/visual_server_wrap_mt.cpp:113
    #4 0x1735539 in OS_X11::initialize(OS::VideoMode const&, int, int) platform/x11/os_x11.cpp:605
    #5 0x18a7d43 in Main::setup2(unsigned long) main/main.cpp:1287
    #6 0x18a27ec in Main::setup(char const*, int, char**, bool) main/main.cpp:1229
    #7 0x1713f70 in main platform/x11/godot_x11.cpp:49
    #8 0x7fdec4c090b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-use-after-free /usr/include/c++/9/bits/atomic_base.h:419 in std::__atomic_base<unsigned int>::load(std::memory_order) const

Chaosus · 2021-04-11T17:19:18Z

I still compiling it for test :S I have only one question why inner_offset instead of inner_radius?

QbieShay · 2021-04-11T17:30:18Z

@Chaosus Oooohhh good point. I didn't follow my own feedback 😅 I will fix it

Chaosus

Worked fine, Do you want to implement this type of shape to CPUParticles?

QbieShay · 2021-04-11T17:35:22Z

@Chaosus yes, that's the plan if the PR is desired. I wanted to have approval before starting to do that because it takes some time

Chaosus · 2021-04-11T17:41:11Z

One more suggestion - maybe changing the axis of rotation of this shape to upward-y is a good idea so the circle will be placed on the ground by the default, rather than the wall - this is cosmetical but I think such portal effects are used more frequently.

QbieShay · 2021-04-11T17:44:31Z

@Chaosus I have oriented the circle to the z axis for compatibility with 2D particles. It isn't the best for 3D but otherwise it doesn't work as-is with 2D particles. Maybe it would be better to have people select an axis (x, y or z, not a custom one), but I'm not sure. What do you think?

Chaosus · 2021-04-11T17:52:01Z

What do you think?

Yeah, I think so - You can rotate the emitter itself but the gravity is still needed to be changed accordingly so provided axis is the best solution, yeah. Maybe a custom axis vector is better than the defined x/y/z axis.

QbieShay · 2021-04-11T18:26:46Z

Okay, will do.

doc/classes/CPUParticles.xml

scene/resources/particles_material.cpp

akien-mga · 2021-05-08T22:35:50Z

Could you amend the commit message to be more explicit? (and remove the `)
E.g. "Implement ring emitter for 3D particles". The "for 3.x" part is not relevant in the commit message, that's conveyed by the branch where the code resides :)

This commits adds a new emitter type for particles material and 3D CPU particles. The new emitter is called "ring" and it can emit either in a ring or cylinder fashion. This adds the following properties for the emitter: 1. ring_emitter_axis: the axis along which the ring/cylinder will be constructed 2. ring_emitter_radius: outer radius of the ring/cylinder 3. ring_emitter_inner_radius: inner radius of the cylinder. when set to zero, particles will emit in the full volume. 4. ring_emitter_height: height of the ring/cylinder emitter. Co-authored-by: Rémi Verschelde <[email protected]>

akien-mga · 2021-05-09T11:33:09Z

No need to credit me as co-author, I didn't do any significant work here :)

QbieShay · 2021-05-09T11:35:56Z

No need to credit me as co-author, I didn't do any significant work here :)

I appreciate getting suggestions directly for the code that i can integrate with a click ;)

akien-mga · 2021-05-09T11:46:38Z

But you deserve the full credit (or blame, if buggy :P) for this contribution ;)
Cosmetic fixes don't really make me an author (including not with regard to copyright).

akien-mga · 2021-05-18T08:54:18Z

Thanks!

QbieShay added topic:rendering topic:shaders topic:3d labels Apr 11, 2021

QbieShay requested review from Chaosus and a team April 11, 2021 16:11

QbieShay force-pushed the ring-emitter-3.x branch from da81705 to 7c6333d Compare April 11, 2021 16:15

Calinou added the enhancement label Apr 11, 2021

Chaosus approved these changes Apr 11, 2021

View reviewed changes

QbieShay force-pushed the ring-emitter-3.x branch from 7c6333d to e2d542d Compare April 11, 2021 17:38

QbieShay force-pushed the ring-emitter-3.x branch from e2d542d to 9dea223 Compare April 12, 2021 21:20

akien-mga added this to the 3.4 milestone Apr 16, 2021

QbieShay force-pushed the ring-emitter-3.x branch from 9dea223 to 4b4661f Compare May 8, 2021 16:52

QbieShay requested a review from a team as a code owner May 8, 2021 16:52

QbieShay force-pushed the ring-emitter-3.x branch 2 times, most recently from dbcfd28 to 2332d60 Compare May 8, 2021 17:20

QbieShay requested a review from a team as a code owner May 8, 2021 17:20

QbieShay changed the title ~~ring emitter for 3.x. the cylinder is aligned with the z axis for compatibility with 2D~~ ring emitter for 3.x. May 8, 2021

QbieShay force-pushed the ring-emitter-3.x branch 3 times, most recently from 9d44048 to 7353f79 Compare May 8, 2021 18:24