InterleavedBuffer: Improve example and docs #12790

Mugen87 · 2017-12-02T13:12:05Z

As the title suggests, a small improvement of docs and example.

WestLangley · 2017-12-02T20:45:24Z

docs/api/core/InterleavedBuffer.html

@@ -11,21 +11,34 @@
 		<h1>[name]</h1>

 		<div class="desc">
+			"Interleaved" means that multiple types of data (position, color etc.) are packed into a single array buffer.
+			Organizing geometry data in this way improves memory locality. By keeping vertex data close together, it is more likely
+			that also caches (e.g. pre-transform vertex cache) store logically related data together. This leads to more cache hits and a better performance.


Nice PR, but maybe you can rephrase or remove these two sentences...

Can you make a suggestion for an alternative? I thought it might be a good Idea to highlight the primary motivation for using interleaved buffers.

I would write it this way:

"Interleaved" means that multiple attributes, possibly of different types, (e.g., position, normal, uv, color) are packed into a single array buffer.

I would leave it at that and avoid a tutorial. Also, avoid making performance claims.

I thought it might be a good Idea to highlight the primary motivation for using interleaved buffers.

What is your reference for the last two sentences? Is that the "primary motivation"?

Okay, i updated the docs with your suggestion. It's okay for me to exclude the other stuff. (For a moment i forgot to focus just on docs and not tutorials 😇).

What is your reference for the last two sentences? Is that the "primary motivation"?

It is mentioned in this literature , Chapter 3, Interleaving your vertex data for improved performance.

From a performance point of view, the preferred way to organize your vertex data is to interleave the data as an array of structures. This is because you get better memory locality for your vertex data. It is likely that when a vertex shader needs the position of a certain vertex, it will also need the normal and texture coordinates for the same vertex at almost the same time. By keeping these coordinates close together in the memory, it is more likely that when one is read into the pre-transform vertex cache, the others will also be read in the same block and will be available in the pre-transform vertex cache when they are needed by the vertex shader.

Well, "primary motivation" is actually my personal wording 😉 . But i think it's okay to say that performance is the main reason when people interleaving vertex data.

looeee · 2017-12-04T05:09:20Z

docs/api/core/InterleavedBuffer.html

 		<h2>Constructor</h2>
 		<h3>[name]( [page:TypedArray array], [page:Integer stride] )</h3>
 		<div>
+			[page:TypedArray array] -- A typed array with a shared buffer. Stores the geometry data.<br/>
+			[page:Integer stride] -- How far apart is the start of each vertex.


How far apart is the start of each vertex

This is worded as a question - can you change it to something like: "The distance between the start of each vertex" or "How far apart the start of each vertex is"

Looks good now 😊

looeee · 2017-12-04T05:09:44Z

docs/api/core/InterleavedBuffer.html

 		</div>

 		<h3>[property:Integer stride]</h3>
 		<div>
+			How far apart is the start of each vertex.


mrdoob · 2017-12-05T08:28:09Z

@looeee looks good?

Mugen87 · 2017-12-05T17:35:39Z

He approved already 😊 #12790 (comment)

WestLangley · 2017-12-05T17:46:30Z

I'm still waiting on a reply... The definition of stride in this PR is not correct.

Mugen87 · 2017-12-05T17:58:42Z

I'm sry, i've missed that. How about:

The number of items that make up a single amount of vertex data.

WestLangley · 2017-12-05T18:11:19Z

Check the WebGL docs for the definition of stride and compare it to our usage. To me, the definition will have to be different. I posted my best guess above.

/ping @benaadams

Mugen87 · 2017-12-05T18:22:21Z

I posted my best guess above.

Sry, but i can't find your guess. Am i blind now?

I would say that the description should be okay now. We are not defining the stride in bytes but in item count. Depending on the type of the item (float, uint8 etc.) we calculate the actual stride via bytesPerElement.

see https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L914

benaadams · 2017-12-05T18:26:13Z

Well. its not necessary the amount of vertex data - but how much to jump from the start of one vertex data to get to the next. e.g. the increment you'd have on a for loop in the last position.

benaadams · 2017-12-05T18:27:03Z

Usually it will be the amount of data per vertex :)

Mugen87 · 2017-12-05T18:30:18Z

I'll pick your definition. I always struggle with this stuff 😅

WestLangley · 2017-12-05T18:33:46Z

Whatever I posted is gone... :/

It was something like:

stride = [ the offset in bytes between the beginning of consecutive vertex attributes ] / bytesPerElement;

That means it is unitless.

This definition should be improved further. Maybe someone can come up with something better.

Mugen87 · 2017-12-07T11:34:14Z

I vote for merging the PR. We can still add a more precise definition of stride with a separate PR.

WestLangley · 2017-12-07T22:28:17Z

I would define InterleavedBuffer.stride like so:

stride - the number of typed-array elements per vertex

Mugen87 · 2017-12-08T19:07:56Z

Great! I'll make a commit 😊

Mugen87 · 2017-12-08T19:08:22Z

Done! 👍

WestLangley · 2017-12-04T17:59:35Z

docs/api/core/InterleavedBuffer.html

 		<h2>Constructor</h2>
 		<h3>[name]( [page:TypedArray array], [page:Integer stride] )</h3>
 		<div>
+			[page:TypedArray array] -- A typed array with a shared buffer. Stores the geometry data.<br/>
+			[page:Integer stride] -- How far apart the start of each vertex is.


How far apart the start of each vertex is.

Can you please check the source code and see if you can verify that the following description is correct?...

The number of elements between the beginning of consecutive vertex attributes.

stride = [ number of bytes between the beginning of consecutive vertex attributes ] / [ bytesPerElement ]

Maybe that description can be reworded, simplified, or corrected, if necessary.

Ah, now i see your original post about stride. I guess you did not submit the review and therefore the post was not visible.

@Mugen87 Yeah, this is all messed up...

WestLangley · 2017-12-07T16:35:34Z

examples/webgl_buffergeometry_points_interleaved.html

-				var ibp = new THREE.InterleavedBuffer( interleaved_buffer_float32, 4 );
-				var ibc = new THREE.InterleavedBuffer( interleaved_buffer_uint8, 16 );
+				var interleavedPositionBuffer = new THREE.InterleavedBuffer( interleavedFloat32Buffer, 4 );
+				var interleavedColorBuffer = new THREE.InterleavedBuffer( interleavedUint8Buffer, 16 );


These are not the "position" and "color" buffers. It would be better to call them interleavedBuffer32 and interleavedBuffer8.

That would become more clear if, for example, there was also normal and uv data in addition to position data. That is, if this example had additional Float32 attributes, you would not refer to the buffer as the "position buffer", because it would contain normal and uv data, too.

For sake of clarity, I would use the names I suggested -- or something similar to those.

I see what you mean. I'll pick your suggestion!

Great. Thanks!

WestLangley · 2017-12-08T22:11:16Z

I think this PR is OK, now.

mrdoob · 2017-12-12T07:12:29Z

Thanks!

InterleavedBuffer: Improve example and docs

d4b8650

WestLangley reviewed Dec 2, 2017

View reviewed changes

InterleavedBuffer: Update docs

1030895

looeee reviewed Dec 4, 2017

View reviewed changes

InterleavedBuffer: Improved wording

459c681

InterleavedBuffer: Improve stride definition

6ac15f3

InterleavedBuffer: Improve stride definition one more time

b1be597

InterleavedBuffer: Improve stride definition one more time

a48b71c

WestLangley approved these changes Dec 8, 2017

View reviewed changes

Examples: Clarify variable names

0b85003

mrdoob merged commit e0cffb1 into mrdoob:dev Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InterleavedBuffer: Improve example and docs #12790

InterleavedBuffer: Improve example and docs #12790

Mugen87 commented Dec 2, 2017

WestLangley Dec 2, 2017

Mugen87 Dec 3, 2017

WestLangley Dec 3, 2017

Mugen87 Dec 3, 2017 •

edited

Loading

looeee Dec 4, 2017

Mugen87 Dec 4, 2017

looeee Dec 4, 2017

looeee Dec 4, 2017

mrdoob commented Dec 5, 2017

Mugen87 commented Dec 5, 2017

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 5, 2017

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 5, 2017 •

edited

Loading

benaadams commented Dec 5, 2017

benaadams commented Dec 5, 2017

Mugen87 commented Dec 5, 2017 •

edited

Loading

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 7, 2017 •

edited

Loading

WestLangley commented Dec 7, 2017

Mugen87 commented Dec 8, 2017

Mugen87 commented Dec 8, 2017

WestLangley Dec 4, 2017

Mugen87 Dec 8, 2017 •

edited

Loading

WestLangley Dec 8, 2017

WestLangley Dec 7, 2017 •

edited

Loading

Mugen87 Dec 8, 2017

WestLangley Dec 8, 2017

WestLangley commented Dec 8, 2017

mrdoob commented Dec 12, 2017

InterleavedBuffer: Improve example and docs #12790

InterleavedBuffer: Improve example and docs #12790

Conversation

Mugen87 commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugen87 Dec 3, 2017 • edited Loading

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

mrdoob commented Dec 5, 2017

Mugen87 commented Dec 5, 2017

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 5, 2017

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 5, 2017 • edited Loading

benaadams commented Dec 5, 2017

benaadams commented Dec 5, 2017

Mugen87 commented Dec 5, 2017 • edited Loading

WestLangley commented Dec 5, 2017

Mugen87 commented Dec 7, 2017 • edited Loading

WestLangley commented Dec 7, 2017

Mugen87 commented Dec 8, 2017

Mugen87 commented Dec 8, 2017

Choose a reason for hiding this comment

Mugen87 Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WestLangley Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WestLangley commented Dec 8, 2017

mrdoob commented Dec 12, 2017

Mugen87 Dec 3, 2017 •

edited

Loading

Mugen87 commented Dec 5, 2017 •

edited

Loading

Mugen87 commented Dec 5, 2017 •

edited

Loading

Mugen87 commented Dec 7, 2017 •

edited

Loading

Mugen87 Dec 8, 2017 •

edited

Loading

WestLangley Dec 7, 2017 •

edited

Loading