IndexBuffer suggestions/improvements (consistency)

Development
1

I’m currently working on creating a generator for quadrilateralized spheres and tried using the IndexBuffer class. Unfortunately, it has some severe shortcomings I’d like to address.

The code I was working with in the beginning was as following:

//creating the buffer
ShortBuffer buffer = BufferUtils.createShortBuffer(indexCount);

// loops omitted for brevity
buffer.put((short) i0).put((short) (i1)).put((short) (i1 + 1));
buffer.put((short) (i0 + 1)).put((short) i0).put((short) (i1 + 1));

// setting the buffer
mesh.setBuffer(Type.Index, 3, buffer);

This obviously falls short if the number of vertices is greater than Short.MAX_VALUE. In that case, an IntBuffer would be appropriate. Behold, IndexBuffer to the rescue! We can create an appropriate IndexByteBuffer, IndexShortBuffer or IndexIntBuffer with it via IndexBuffer.createIndexBuffer(vertexCount, indexCount);

Or can we? The method never generates an IndexByteBuffer. It only generates the two bigger types. This makes me wonder why an IndexByteBuffer class exists in the first place. Consistency is king, the method should also generates IndexByteBuffer. The wrapIndexBuffer method does already correctly wrap ByteBuffer into IndexByteBuffer!

Moving on from this, after having created an IndexBuffer with IndexBuffer.createIndexBuffer(vertexCount, indexCount);, there is no way to set it on a mesh. This becomes obvious when looking at jme-internal code, for example found in WrappenIndexBuffer.convertToList(Mesh mesh), lines 110 and following:

        if (outBuf instanceof IndexIntBuffer){
            mesh.setBuffer(Type.Index, 3, (IntBuffer)outBuf.getBuffer());
        }else{
            mesh.setBuffer(Type.Index, 3, (ShortBuffer)outBuf.getBuffer());
        }

This can’t be the way to set an index buffer, having to inspect the type in order to be able to do it. The Mesh class desperately needs a new method, either setIndexBuffer or setBuffer(type type, int components, IndexBuffer buffer), and this kind of introspection needs to be pushed down there. User code shouldn’t have to inspect types!

Last but not least, classes like ShortBuffer offer fluid interfaces. You can do buffer.put(0f).put(f).put(2f); and so on with those buffers. IndexBuffer only offers a void method for it. It would be very desirable if the IndexBuffer class would offer the same interface in order to facilitate easier swapping to it!

The code I already had is presented at the top of this post. This is the code I expected to be able to get:

//creating the buffer
IndexBuffer buffer = IndexBuffer.createIndexBuffer(vertexCount, indexCount);

// loops omitted for brevity
buffer.put(i0).put(i1).put(i1 + 1);
buffer.put(i0 + 1).put(i0).put(i1 + 1);

// setting the buffer
mesh.setBuffer(Type.Index, 3, buffer);

This is the code I actually need:

// creating the buffer
IndexBuffer indexBuffer = IndexBuffer.createIndexBuffer(vertexCount, indexCount);

// loops omitted for brevity
indexBuffer.put(c++, i0);
indexBuffer.put(c++, i1);
indexBuffer.put(c++, i1+1);
					
indexBuffer.put(c++, i0+1);
indexBuffer.put(c++, i0);
indexBuffer.put(c++, i1+1);

//setting the buffer
if (indexBuffer instanceof IndexByteBuffer)
    mesh.setBuffer(Type.Index, 3, (ByteBuffer) buffer.getBuffer());
else if (indexBuffer instanceof IndexShortBuffer)
    mesh.setBuffer(Type.Index, 3, (ShortBuffer) buffer.getBuffer());
else if (indexBuffer instanceof IndexIntBuffer)
    mesh.setBuffer(Type.Index, 3, (IntBuffer) buffer.getBuffer());

The code needed to swap away from a specific buffer (Byte-, Short- or IntBuffer) over to a generic IndexBuffer is much longer and much more complicated.

In short, the three improvements I am advising are:

  • IndexBuffer.createIndexBuffer(vertexCount, indexCount); should also return IndexByteBuffer,
  • Mesh should accept an IndexBuffer so that user-code does not need to do type inspection and
  • IndexBuffer should offer the same fluid interface as the other buffers, especially in put, returning itself instead of void.

At the same time it could be an opportunity to swap existing JME code which still utilizes other buffers directly over to IndexBuffer.

If these changes are wanted, I could potentially work on providing a PR for them.

4 Likes
2

Since we are talking about vertex buffers and we have a class VertexBuffer that is ultimately used to encapsulate all those types, i would deprecate all those methods and keep only setBuffer(VertexBuffer) and then have the helper classes, that generate vertexbuffers, output to VertexBuffer.
Obviously in the current state the vertexbuffer class is a bit inconvenient to use, for example we have only one constructor that sets only some stuff, that would have to change.

1 Like
3

I agree that createIndexBuffer() should create a byte buffer whenever possible. The other 2 changes seem desirable as well. I encourage you to submit a PR.

4

@sgold After having taken a closer look at the implementation of Mesh, I have found an better way to achieve the goal. Instead of adding another method to Mesh, we can leverage Mesh#setBuffer(Type type, int components, Format format, Buffer buf). It already accepts a generic java.nio.Buffer. I can add a IndexBuffer#getFormat() method that provides the format and have the Byte-Short- and Int implementations of IndexBuffer return the appropriate format. this would confine all changes to IndexBuffer and its subclasses, would achieve the goal as simplyfying the APi but not introduce chages to Mesh.

@RiccardoBlb its a good idea in the long run, but requires far more changes. For now I’d like to keep the change small, with as much benefit as possible. I might be able to introduce larger changes to jme in the future.

4 Likes
5

I’ve got the changes done and will submit a PR shortly. So far I have only identified one use where JME-internal code relied on IndexBuffer.createIndexBuffer(vertexCount, indexCount); creating exactly two kinds of buffers, I have fixed that as well and ported it to the new method. I’m currently running the tests to see if I may have missed something.

I have already spotted several points inside jme where code can be improved by switching. For example in Mesh#extractVertexData(Mesh other), in Line 1271 - 1276 the code of IndexBuffer#createIndexBuffer is copied exactly as-is, and in line 1289 type inspection is again used to determine thy format of the buffer. This could be simplified greatly (Line 1271-1276 would become one line, in line 1289 the conditional would be eliminated) by leveraging the power of the new methods in IndexBuffer.

1 Like
6

There I’ll vote for setBuffer(type type, int components, IndexBuffer buffer) instead of setIndexBuffer.
And please no setBuffer(int components, IndexBuffer buffer) so that one could actually even use the IndexBuffer tool to have smaller vertex buffers or something else.

About Mesh#setBuffer(Type type, int components, Format format, Buffer buf), I’d rather have the above, because then you set every buffer with the same syntax and don’t need a special one for the index buffer. This would lead to people still rather cast, because they don’t know that they need a different method for that to work. Like it would work for your use case, but it wouldn’t be consistent and people would most likely overlook it

7

You realize that this method already exists? The changes I’ve made in my branch are confined to IndexBuffer (and its subclasses).

I’ve deliberately avoided introducing changes to Mesh at this point, because as @RiccardoBlb says, it would be better to swap this completely over to VertexBuffer at some point in the future, and the more changes to Mesh are introduced now, the more methods need to be deprecated/changed/removed in the future.

The code I’m using right now looks like this:

//creating the buffer
IndexBuffer buffer = IndexBuffer.createIndexBuffer(vertexCount, indexCount);

// loops omitted for brevity
buffer.put(i0).put(i1).put(i1 + 1);
buffer.put(i0 + 1).put(i0).put(i1 + 1);

// setting the buffer
mesh.setBuffer(Type.Index, 3, buffer.getFormat(), buffer.getBuffer());

That pretty much achieves the goal of getting rid of type inspection.

Adding another method to Mesh to simplify this further is something that can still be added later if desired. I’m not exactly against it (in fact, it was one of the things I initially considered as well), but I’m also not sure its needed at this point.

I’ll make a PR in the evening, we can then discuss the changes directly with the code changes I’m proposing at hand.

PR is found here:

3 Likes
8

+1

9

Yeah, I got that, but like you, I didn’t know about it in the beginning and most code doesn’t use it probably either. Actually probably even the wiki for custom meshes isn’t using it (or indexbuffer at all, though).

So, if there was this new method, I would’ve found it probably by code completion.
But your objection is sound, provided this future rewrite will ever take place at all

10

The wiki uses int[] and BufferUtils#createIntBuffer(int[]). JME internally uses IndexBuffer rarely as well (which is a shame, because a lot of the functionality is repeated elsewhere).

I’m really torn between wanting to update more code in JME to IndexBuffer or starting on a larger VertexBuffer rewrite.

2 Likes