Custom vertex format support for ParticleFX components #7866

Jhonnyg · 2023-08-07T06:47:53Z

Particle FX nodes can now utilise the custom vertex formats previously added to sprites. This enables you to pass custom data from the editor into shader attributes when rendering particles.

Fixes #7818

PR checklist

Jhonnyg · 2023-08-16T08:30:50Z

editor/src/clj/editor/gl/vertex.clj

-(defn vertex-overlay
-  "Use a vertex layout together with an existing ByteBuffer. Returns a vertex buffer suitable
-   for the `use-with` function.
-
-   This will assume the ByteBuffer is an integer multiple of the vertex size."
-  [layout ^ByteBuffer buffer]
-  (assert layout)
-  (assert (= 0 (mod (.limit buffer) (:vertex-size layout))))
-  (let [^ByteBuffer buffer (.duplicate buffer)
-        limit         (.limit buffer)
-        count         (mod limit (:vertex-size layout))
-        buffer-starts (buffer-starts limit layout)
-        slices        (b/slice buffer (map min (repeat limit) buffer-starts))]
-    (->PersistentVertexBuffer layout count buffer slices (AtomicLong. count) not-allowed not-allowed)))
-


No used anywhere now

Jhonnyg · 2023-08-16T08:38:13Z

engine/gamesys/src/gamesys/components/comp_particlefx.cpp

-        uint32_t buffer_size = dmParticle::GetVertexBufferSize(ctx->m_MaxParticleCount, dmParticle::PARTICLE_GO);
+        // position   : 3
+        // color      : 4
+        // texcoord0  : 2
+        // page_index : 1
+        const uint32_t default_vx_size = sizeof(float) * (3 + 4 + 2 + 1);
+        const uint32_t buffer_size     = ctx->m_MaxParticleCount * 6 * default_vx_size;
+        world->m_VertexBufferData.SetCapacity(ctx->m_MaxParticleCount * 6 * default_vx_size);


We don't have any particle "formats" now, the size of the vx buffer is only dependent on the vertex size.

engine/gamesys/src/gamesys/components/comp_particlefx.cpp

Jhonnyg · 2023-08-28T11:20:26Z

engine/graphics/src/wscript

+    obj = bld.stlib(features        = obj.features + ['skip_asan'],
+                    includes        = obj.includes,
+                    source          = bld.path.ant_glob(['graphics_proto.cpp']) + bld.path.parent.ant_glob('proto/graphics/*'),
+                    protoc_includes = obj.protoc_includes,
+                    use             = obj.use,
+                    target          = 'graphics_proto_noasan')


Particle now depends on graphics being built first, since we use data structures from that library when producing particles.

editor/src/clj/editor/particle_lib.clj

engine/particle/src/particle.cpp

Jhonnyg · 2023-08-29T11:44:00Z

engine/particle/src/particle.cpp

+                case dmGraphics::VertexAttribute::SEMANTIC_TYPE_COLOR:
+                {
+                    if (info.m_NameHash == VERTEX_STREAM_COLOR)
+                    {
+                        memcpy(write_ptr, &color, info.m_ValueByteSize);
+                    }
+                    else
+                    {
+                        memcpy(write_ptr, info.m_ValuePtr, info.m_ValueByteSize);
+                    }
+                } break;


As previously mentioned, we need to know if the stream has the name "color" or not, if that is the case we output out the particle color and otherwise we put the binary value from the actual attribute.

Isn't this true for the other cases?
If the user set a POSITION or TEXCOORD attribute with name "extra", it will get the predefined position/uv values.
Or isn't is allowed, and should that a build error?

Also, do we want to look at the type?
E.g. if the user really wants to use a single uint32_t instead of float4 for color for bandwidth.

Later, I also feel this "write logic" should be standardized/centralized somehow.

Yeah that's true I suppose. The problem I ran into was that the "color" semantic is the only one that has any value in the editor since we decorate the property with a color widget. So the way we had originally setup the semantic types means that I can't decorate an attribute with a color widget, but maybe that's fine and should be a separate "edit type" value or something.. But yes, if we are to conform these vertex writing functions into a shared location I think we should produce the built-in data based on the semantic types.

Also, do we want to look at the type?
E.g. if the user really wants to use a single uint32_t instead of float4 for color for bandwidth.

Do you mean with the built-in semantic types? Because the data produced from the content pipeline is narrowed or expanded to the correct type, but the built-in types are not. For the built-in types we need to do data conversion between the floats (it's all float streams I think?) into whatever format. I think we should do it, but I'd rather try to get this PR in first and then take a stab at merging the vertex writing function in a separate PR.

Jhonnyg · 2023-08-29T11:44:30Z

engine/particle/src/particle.cpp

+            Vector3 p0_local;
+            Vector3 p1_local;
+            Vector3 p2_local;
+            Vector3 p3_local;
+
+            if (use_local_position)
+            {
+                p0_local = -x - y;
+                p1_local = -x + y;
+                p2_local = x - y;
+                p3_local = x + y;
+            }


These will only be used if there is a stream with position_local semantic being used.

Jhonnyg · 2023-08-29T11:45:09Z

engine/particle/src/particle.cpp

-                const char* config_key = MAX_PARTICLE_COUNT_KEY;
-
-                if (format == PARTICLE_GUI)
-                    config_key = "gui.max_particle_count";
-
-                dmLogWarning("Maximum number of particles (%d) exceeded, particles will not be rendered. Change \"%s\" in the config file.", context->m_MaxParticleCount, config_key);
+                res = GENERATE_VERTEX_DATA_MAX_PARTICLES_EXCEEDED;


Each component has to deal with the error by themselves, since we don't know the source of what's being generated.

Jhonnyg · 2023-08-29T21:28:00Z

engine/particle/src/particle.cpp

+        if (ix >= context->m_AttributeDataPtrs.Capacity())
+        {
+            void* new_ptr = malloc(ATTRIBUTE_WRAPPER_SIZE);
+            context->m_AttributeDataPtrs.OffsetCapacity(1);
+            context->m_AttributeDataPtrs.Push(new_ptr);
+            return new_ptr;
+        }


We need to retain the same pointers between every simulation step, which other expanding array solutions will not guarantee, and I don't think we want to preallocate a large array for this since the scratch buffer approach is ONLY for the editor.

matgis

Found some minor niggles on the editor side, but they shouldn't be a lot of effort to fix!

editor/src/clj/editor/particle_lib.clj

editor/src/clj/editor/particlefx.clj

editor/src/clj/editor/graphics.clj

editor/src/clj/editor/particle_lib.clj

matgis · 2023-08-31T12:30:17Z

editor/src/clj/editor/particlefx.clj

+              context (:context pfx-sim)
+              vbuf (vtx/wrap-vertex-buffer vertex-description :static raw-vbuf)
+              all-raw-vbufs (vset (:raw-vbufs pfx-sim) emitter-index raw-vbuf)]
+          (swap! pfx-sim-atom assoc :raw-vbufs all-raw-vbufs)


I'm a bit annoyed that we have to do it like this because the vertex-description is not available until we have a GL context to compile the shaders with.

Perhaps in the future we could get the information from the output of the SPIR-V compiler and prepare the buffers before rendering?

Oh well. Some day...

Yeah, it makes things more complicated for sure... But hm, I'll add a note about this for now and it will have to be a headache for the future I guess 🤷

editor/src/clj/editor/sprite.clj

JCash

Mainly the assert in the editor only C++ code needs attention.

Also some questions that may warrant some clarifications.

engine/gamesys/src/gamesys/components/comp_gui.cpp

JCash · 2023-09-06T07:42:59Z

engine/gamesys/src/gamesys/components/comp_gui.cpp

@@ -1060,7 +1081,7 @@ namespace dmGameSystem
        dmGui::NodeType node_type = dmGui::GetNodeType(scene, first_node);
        assert(node_type == dmGui::NODE_TYPE_PARTICLEFX);

-        uint32_t vb_max_size = dmParticle::GetMaxVertexBufferSize(gui_world->m_ParticleContext, dmParticle::PARTICLE_GUI) - gui_world->m_RenderedParticlesSize;
+        uint32_t vb_max_size = dmParticle::GetMaxVertexBufferSize(gui_world->m_ParticleContext, sizeof(ParticleGuiVertex)) - gui_world->m_RenderedParticlesSize;


Not sure what the best way is to define the particle budget now.
Previously it was bytes, but now that we can support multiple materials with different vertex formats, it might be relevant to think of #particles.
🤔

The usage of the sizeof(ParticleGuiVertex) seems a little strange now. Perhaps add a comment or two to show that the size is an estimate, and that the code is works ok?

E.g. the vertex count relies on that size

Not sure what the best way is to define the particle budget now.
Previously it was bytes, but now that we can support multiple materials with different vertex formats, it might be relevant to think of #particles.
🤔

What budget are you referring to here? Both the GUI and the particle settings specify particle count (and components)

The usage of the sizeof(ParticleGuiVertex) seems a little strange now. Perhaps add a comment or two to show that the size is an estimate, and that the code is works ok?

E.g. the vertex count relies on that size

But for GUI the size isn't an estimate, it is what is allocated regardless of what material is used 🤔 this is because we don't support any custom vertex formats in GUI right now, so we don't use the configuration from the material.

engine/gamesys/src/gamesys/components/comp_particlefx.cpp

JCash · 2023-09-06T07:59:04Z

engine/gamesys/src/gamesys/components/comp_particlefx.cpp


-        uint32_t ro_vertex_count = vb_end - vb_begin;
-        vertex_buffer.SetSize(vb_end - vertex_buffer.Begin());
+        vertex_buffer.SetSize(vb_size);


It's not a new pattern in the engine, but I think as communications go, we could make it clearer that between the SetCapacity() and SetSize() the array size is 0 and it's not really ok to write to the memory.

E.g. this could be changed to:

SetCapacity(max_size); SetSize(max_size); new_size = ...do work; SetSize(new_size);

It's not a big deal in this case, but I recall having to change other places where that logic becomes problematic.

JCash · 2023-09-06T08:13:04Z

engine/particle/src/particle.cpp

+                case dmGraphics::VertexAttribute::SEMANTIC_TYPE_COLOR:
+                {
+                    if (info.m_NameHash == VERTEX_STREAM_COLOR)
+                    {
+                        memcpy(write_ptr, &color, info.m_ValueByteSize);
+                    }
+                    else
+                    {
+                        memcpy(write_ptr, info.m_ValuePtr, info.m_ValueByteSize);
+                    }
+                } break;


Isn't this true for the other cases?
If the user set a POSITION or TEXCOORD attribute with name "extra", it will get the predefined position/uv values.
Or isn't is allowed, and should that a build error?

Also, do we want to look at the type?
E.g. if the user really wants to use a single uint32_t instead of float4 for color for bandwidth.

Later, I also feel this "write logic" should be standardized/centralized somehow.

engine/particle/src/particle.cpp

Jhonnyg and others added 18 commits July 21, 2023 11:12

Initial engine support

358c4b6

Correct index offset into draw calls

2ebc81c

Remove particle format from particle lib

1034ce3

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

6b2ac59

Fix gui tests

58ab29d

Fix stats

349873c

Editor support work-in-progress

d50d773

Use noasan version of graphics-proto for particles

5cdda55

Fix particle test + particle save/build values

adc8a6e

Editor preview works now

d151858

Small build fix

a82749f

Fix particle tests

c503143

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

f2807b0

Another fix

66ef3b4

GUI Fixes

086ec7b

Conform sprite and pfx helpers into graphics namespace

8cb30ec

More editor cleanup

362462d

Fix particlefx widget rendering in editor

decf559

Jhonnyg commented Aug 16, 2023

View reviewed changes

engine/gamesys/src/gamesys/components/comp_particlefx.cpp Outdated Show resolved Hide resolved

Jhonnyg added 7 commits August 18, 2023 15:01

Pass in vx size when creating the buffer

1890eef

Pass in vertex size instead

9bf53ee

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

997114c

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

b8ed80b

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

c15bcbf

Editor test fixes

51ef462

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

9002105

Jhonnyg commented Aug 28, 2023

View reviewed changes

Use semantic type

9a3f8e0

Jhonnyg added 2 commits August 29, 2023 10:47

Produce local vertices

91afc45

Fix editor preview again

845e065

Jhonnyg commented Aug 29, 2023

View reviewed changes

editor/src/clj/editor/particle_lib.clj Outdated Show resolved Hide resolved

Jhonnyg commented Aug 29, 2023

View reviewed changes

editor/src/clj/editor/particle_lib.clj Outdated Show resolved Hide resolved

Jhonnyg commented Aug 29, 2023

View reviewed changes

engine/particle/src/particle.cpp Show resolved Hide resolved

Jhonnyg commented Aug 29, 2023

View reviewed changes

engine/particle/src/particle.cpp Outdated Show resolved Hide resolved

Jhonnyg commented Aug 29, 2023

View reviewed changes

Jhonnyg added 2 commits August 29, 2023 15:28

Fix preview rendering by refactoring scratchbuffer usage

42a36f5

pre-review fixes

2d9ff99

Jhonnyg added the feature request A suggestion for a new feature label Aug 29, 2023

Jhonnyg added 3 commits August 29, 2023 22:53

The editor preview should be correct for more than one attribute now

dc42dca

Fix engine tests

8f10fae

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

f1141ba

Jhonnyg changed the title ~~Issue 7818 custom vertex formats particlefx~~ Custom vertex format support for ParticleFX components Aug 29, 2023

Jhonnyg commented Aug 29, 2023

View reviewed changes

Jhonnyg requested review from JCash and matgis August 29, 2023 21:28

Jhonnyg marked this pull request as ready for review August 29, 2023 21:28

matgis requested changes Aug 31, 2023

View reviewed changes

JCash requested changes Sep 6, 2023

View reviewed changes

Jhonnyg added 3 commits September 7, 2023 15:51

Merge branch 'dev' into issue-7818-custom-vertex-formats-particlefx

d4c0343

Review fixes pt 1

e8c5df4

Review fix 2

1ba6429

Jhonnyg merged commit ebc50ab into dev Sep 8, 2023
22 checks passed

Jhonnyg deleted the issue-7818-custom-vertex-formats-particlefx branch September 8, 2023 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom vertex format support for ParticleFX components #7866

Custom vertex format support for ParticleFX components #7866

Jhonnyg commented Aug 7, 2023 •

edited

Jhonnyg Aug 16, 2023

Jhonnyg Aug 16, 2023 •

edited

Jhonnyg Aug 28, 2023 •

edited

Jhonnyg Aug 29, 2023

JCash Sep 6, 2023

Jhonnyg Sep 7, 2023

Jhonnyg Aug 29, 2023

Jhonnyg Aug 29, 2023 •

edited

Jhonnyg Aug 29, 2023

matgis left a comment

matgis Aug 31, 2023

Jhonnyg Sep 7, 2023

JCash left a comment

JCash Sep 6, 2023

JCash Sep 6, 2023

Jhonnyg Sep 7, 2023

JCash Sep 6, 2023

JCash Sep 6, 2023

Custom vertex format support for ParticleFX components #7866

Custom vertex format support for ParticleFX components #7866

Conversation

Jhonnyg commented Aug 7, 2023 • edited

PR checklist

Choose a reason for hiding this comment

Jhonnyg Aug 16, 2023 • edited

Choose a reason for hiding this comment

Jhonnyg Aug 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jhonnyg Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matgis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jhonnyg commented Aug 7, 2023 •

edited

Jhonnyg Aug 16, 2023 •

edited

Jhonnyg Aug 28, 2023 •

edited

Jhonnyg Aug 29, 2023 •

edited