Issue with VertexDraw

[rainergericke]

Hi all,

I found an issue, when I want to draw a line strip with VertexDraw. My idea was to get rid of the indices vector, but it doesn't work. With a VertexIndexDraw the result is as expected. This is my code (working config):

auto stategraph = createLineStateGroup(m_options,  VK_PRIMITIVE_TOPOLOGY_LINE_STRIP);

bool use_index_draw = true;
if(use_index_draw) {
    // setup vertex index draw
    auto vid = vsg::VertexIndexDraw::create();
    
    vsg::DataList arrays;
    arrays.push_back(vertices);
    arrays.push_back(colors);
    
    vid->assignArrays(arrays);
    
    vid->assignIndices(indices);
    vid->indexCount = (uint32_t)indices->size();
    
    vid->instanceCount = 1;
    stategraph->addChild(vid);
} else {
    // setup vertex index draw
    auto vd = vsg::VertexDraw::create();
    
    vsg::DataList arrays;
    arrays.push_back(vertices);
    arrays.push_back(colors);
    
    vd->assignArrays(arrays);
    vd->instanceCount = 1;
    
    stategraph->addChild(vd);
}

transform->addChild(stategraph);

Expected result is:

VertexDraw gives this:

Did I miss a setting?

[robertosfield]

Did I miss a setting?

Looks like it. You'll kick yourself, but rather than provide the answer directly I'll link to the relevant code:

https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/nodes/VertexDraw.h#L36

Note all the = 0 defaults, some are fine as 0, others are not... :-)

[andre-caldas]

Note all the = 0 defaults, some are fine as 0, others are not... :-)

I know almost nothing about VSG. But the need to set those values by hand (and using a cast!?) bugs me.

What I can understand from this is that probably indices->size() is not always the number of indices in indices. So, this information gets lost somewhere and the programmer has to keep the lost information stored somewhere else.

I would say that people will simply (carelessly) use indices->size() every time. So, this is not only error prone. It is also seems very hard to assert and detect. :-(

[robertosfield]

The uint32_t usage for the vertexCount, instanceCount etc. is done because this is exactly what the underlying Vulkan API uses, as much as possible the VSG tries to map 1:1 to Vulkan.

For containers like vsg::Array there is size_t size() member to provide similar API to std::vecrtor, here the VSG straddles the conventions of two APIs Vulkan & C++ standard. This can occasionally be a slightly awkward but sometimes there isn't a single solution that can fit all purposes perfectly. The VSG is far from alone with having to juggle these types of issues.

[andre-caldas]

The "cast" thing was just a side note to my comment. Passing a range that does not know its own size and later setting some internal indexCount, with or without the "cast" is already a problem.

The cast is a consequence of the design. The design violates the P1 core guideline, IMO. Although it is a very subjective guideline... :-)

If the cast is a "bad thing", then the guidelines say it should be encapsulated:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ri-encapsulate

If indices does not know its own size, assignIndices could take a second parameter. This would avoid both: "casting" and "forgetting to set indexCount".

But I still do not understand... indices does not know its own size? Does it happen that indices->size() is not what we want to assign to indexCount? When does this happen?

[robertosfield]

I'm familiar with CppCoreGuidelines but software engineering is a balance of factors, sometimes there is not right answer.

You lack of knowledge of Vulkan/VSG is hindering your appreciation of the various ways the different classes can be used. The VSG doesn't attempt to constrain you to just the subset that you currently seem to have in your head. It's prefectly valid usage for VeretxIndexDraw::indexCount to be different from the indices->size(). One index array can be shared across many VertexIndextDraw/Geometry/BindIndexBuffer.

These classes can even be used with a null indices array, with the index buffers contents residing purely in GPU memory - if you look at the implementation you'll find that it uses BufferInfo rather than hardwired to array.

So please don't fixated on your particular interpretations of how VSG classes are used, there are general purpose classes that are designed to be used in many different ways for different tasks. This flexibility does mean there is certain level of complexity that can't be hidden from users, this is something largely dictated by Vulkan and how GPUs work, the VSG tries to make this as easy as possible but there is no magic wand that can hide all complexity while retaining the power/flexibility that advanced users need.

[andre-caldas]

You lack of knowledge of Vulkan/VSG is hindering your appreciation of the various ways the different classes can be used.

Yes, I think so! I know nothing about GPU, Vulkan, etc. :-)
I am very happy that VSG's tutorial is evolving a lot lately.

So please don't fixated on your particular interpretations of how VSG classes are used, there are general purpose classes that are designed to be used in many different ways for different tasks.

I actually do not know anything about VSG classes. In the process of learning, I see things that make me wonder. I am standing from a very generic point of view... unaware of the real difficulties behind decisions. For that reason, I ask. :-)

Thank you for answering me and for your patience! I still have doubts and opinions on this matter although I do not know much. With my flawed point of view, I still tend to think that it would be better if assignIndices() took a second argument so to update indexCount, making it always coherent with buffer contents and programmers intentions. But I am unaware of things that could make it not work properly... for example, maybe setting indexCount would trigger some action in a different thread... I don't know...

But anyway, I do not intend to "hijack" this thread or to take too much of your time arguing with a "clueless pedantic noob" over decisions that are long discussed and consolidated. :-D

Thank you for the great work in VSG. I hope I can get it to work in my pet project very soon.

[rainergericke]

Bingo! vd->vertexCount = numPoints; does the job.

Thanks