Polished BatchRenderer #5852
Merged
Polished BatchRenderer #5852
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I have edited the BatchRenderer class. The following major changes were made: 1. Changed old `vertexCount` to `_flushId`. This is because the name itself was misleading. It signified the number of flushes in the current frame. 2. Renamed `vaos` and `vaosMax` to `_packedGeometries` and `_packedGeometryPoolSize`. They _were not_ VAOs in the sense that they binded all the buffers automatically to the WebGL context. Instead, they were just reusable `BatchGeometry` objects. 3. Renamed `groups` to `_drawCalls`. That is what they are, and it makes more sense, especially after documenting it. 4. Renamed all private variables by adding a _ prefix. 5. Added documentation to all private/public properties and methods. 6. Fixed destroy(). It was nullifying non-existent variables. They were probably from when this class was being built/tested. I've fixed it so that it nullifies the correct resources. 7. contextChange signal is now hooked in `BatchRenderer` rather than the `BatchPluginFactory#create`. This makes sense since the former is the one implementing the `contextChange` signal. 8. SIDE EFFECT: Graphics used the `shader` property of `BatchRenderer`, and since that was deemed a private properties, it broke. It fixed that by making Graphics use `_shader`. 9. I reordered the way in which properties are initialized in the constructor. The most important/public ones are on the top. They are ordered in a way that makes a new-comer sense. 10. Reordered methods. The first methods are related to rendering and are lifecycle-type methods: contextChange, render, flush, and destroy. Then the start and stop methods. Then the private methods getAttributeBuffer, getIndexBuffer, and packInterleavedGeometry. 11. Renamed packGeometry to packInterleavedGeometry; this is because the method doesn't take any geometry to put into. Rather it takes the interleaved buffers. 12. Documented literally anything else.
Codecov Report
@@ Coverage Diff @@
## dev #5852 +/- ##
==========================================
- Coverage 70.53% 70.52% -0.01%
==========================================
Files 205 205
Lines 10406 10403 -3
==========================================
- Hits 7340 7337 -3
Misses 3066 3066
Continue to review full report at Codecov.
|
ivanpopelyshev
approved these changes
Jun 30, 2019
englercj
approved these changes
Jun 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(PLEASE JUST READ THE NEW
BatchRendererCLASS, since the changes reordered a lotof things and you might not be able to locate the additions/deletions pairs.
I have edited the BatchRenderer class. The following major changes were
made:
Changed old
vertexCountto_flushId. This is because the nameitself was misleading. It signified the number of flushes in the
current frame.
Renamed
vaosandvaosMaxto_packedGeometriesand_packedGeometryPoolSize. They were not VAOs in the sensethat they binded all the buffers automatically to the WebGL
context. Instead, they were just reusable
BatchGeometryobjects.Renamed
groupsto_drawCalls. That is what they are, and itmakes more sense, especially after documenting it.
Renamed all private variables by adding a _ prefix.
Added documentation to all private/public properties and methods.
Fixed destroy(). It was nullifying non-existent variables. They
were probably from when this class was being built/tested. I've fixed
it so that it nullifies the correct resources.
contextChange signal is now hooked in
BatchRendererrather thanthe
BatchPluginFactory#create. This makes sense since the formeris the one implementing the
contextChangesignal.SIDE EFFECT: Graphics used the
shaderproperty ofBatchRenderer,and since that was deemed a private properties, it broke. It fixed that
by making Graphics use
_shader.I reordered the way in which properties are initialized in the
constructor. The most important/public ones are on the top. They are
ordered in a way that makes a new-comer sense.
Reordered methods. The first methods are related to rendering and
are lifecycle-type methods: contextChange, render, flush, and destroy. Then
the start and stop methods. Then the private methods getAttributeBuffer,
getIndexBuffer, and packInterleavedGeometry.
Renamed packGeometry to packInterleavedGeometry; this is because
the method doesn't take any geometry to put into. Rather it takes
the interleaved buffers.
Documented literally anything else.
Description of change
Pre-Merge Checklist
npm run lint)npm run test)