Transform refactor #1221

1chandu · 2019-08-28T19:49:53Z

Background

Phase#1 changes as described in RFC
This is still WIP, I will be collecting team's feedback on RFC and update this PR.
Verified using existing Transform unit tests and core/transform example.
Remaining items:

Addressing any feedback from RFC
Docs
Unit tests specific to new modules/classes.

Change List

[POC] Transform refactor

ibgreen

This is a lot to review, I will certainly need more time, but some quick feedback/ideas:

perhaps writing/updating markdown docs as you go could help reviewers a lot. Start by reviewing the doc changes could guide the code review.

One question with this design (I need to reread the RFC) is whether the user can combine buffers and textures in one call, or whether it always uses one type.

I count ~800 lines, so this is class is getting pretty big, and I suspect it will keep growing.

Perhaps at some point there is a bigger GPGPU compute system growing out of this and we can carve out a small Transform class to keep inside core?

It raises the question whether this should be kept in core (it probably should stay in core, but I think we want to keep a close eye on module growth).

1chandu · 2019-08-29T16:35:01Z

perhaps writing/updating markdown docs as you go could help reviewers a lot.

I did write how these classes work and how they interact in RFC, I will write docs after collecting first round of feedback on overall approach.

One question with this design (I need to reread the RFC) is whether the user can combine buffers and textures in one call, or whether it always uses one type.

The code as is, yes you can combine buffer and textures in one call. I discussed few options in RFC for Phase#2, some options support and some options don't, but this PR doesn't cover Phase#2, just lays foundations for it.

I count ~800 lines, so this is class is getting pretty big, and I suspect it will keep growing.
It raises the question whether this should be kept in core (it probably should stay in core, but I think we want to keep a close eye on module growth).

Yep, these are the goals as describe in RFC, to divide this class and move some components out of core module, more details in RFC.

modules/core/src/lib/transform/buffer-transform-binding.js

modules/core/src/lib/transform/buffer-transform.js

modules/core/src/lib/transform/texture-transform.js

Pessimistress · 2019-09-03T19:13:06Z

modules/core/src/lib/transform/transform.js

+  delete() {
+    this.model.delete();
+    /* eslint-disable no-unused-expressions */
+    this.bufferTransform && this.bufferTransform.delete();


Nit: use if

Pessimistress · 2019-09-03T19:15:02Z

modules/core/src/lib/transform/transform.js

+    if (opts.elementCount) {
+      this.model.setVertexCount(opts.elementCount);
+    }
+    const resourceTransforms = [this.bufferTransform, this.textureTransform].filter(Boolean);


Make a helper function?

modules/core/src/lib/transform/transform.js

1chandu · 2019-09-07T23:18:36Z

Addressed RFC Feedback :

Merge BufferTransformBinding into BufferTransform and TextureTransformBinding into TextureTransform
Add docs for BufferTransform and TextureTransform, didn't link them to website yet, since these are still private internal classes, added for easier review.
Address other review comments.

1chandu · 2019-09-07T23:28:12Z

docs/api-reference/core/transform/texture-transform.md

+
+## Methods (Model props)
+
+### getDrawOptions(opts: Object) : Object


I feel, updateDrawOptions is more suitable, here and for BufferTransform.

It's quite awkward how the props/options are passed from the parent class to the child then back again. Shouldn't BufferTransform and TextureTransform own their models instead of Transform?

as discussed offline, these are internal methods to Transform, I will address these concerns in Phase-2, where I am planning to make this API public.

Pessimistress · 2019-09-09T19:12:37Z

docs/api-reference/core/transform/buffer-transform.md

+  * `feedbackBuffers` (`Object`, Optional) - key and value pairs, where key is the name of vertex shader varying and value is the corresponding `Buffer` object or buffer params object. If a buffer params object is specified, it will contain following fields, these can be used to capture data into the buffer at particular offset and size.
+    * `buffer`=(Buffer) - Buffer object to be bound.
+    * `byteOffset`=(Number, default: 0) - Byte offset that is used to start recording the data in the buffer.
+    * `byteSize`=(Number, default: remaining buffer size) - Size in bytes that is used for recording the data.


Maybe not for this PR: Can we follow the same accessor convention that model.setAttributes uses?

Pessimistress · 2019-09-09T19:18:08Z

docs/api-reference/core/transform/texture-transform.md

+
+## Methods (Model props)
+
+### getDrawOptions(opts: Object) : Object


It's quite awkward how the props/options are passed from the parent class to the child then back again. Shouldn't BufferTransform and TextureTransform own their models instead of Transform?

coveralls · 2019-09-09T23:38:24Z

Coverage increased (+4.6%) to 67.229% when pulling 7b6cbb0 on r/TransformRefactor into 745947d on master.

* master: Transform refactor (#1221) dev-tools: Bump to 0.0.29 (#1232) v7.3.0-alpha.7 Transform: Fix update of Buffers and elementCount. (#1224) point docs to release branch (#1229) update website fonts (#1227)

1chandu requested review from tsherif and Pessimistress August 28, 2019 19:49

ibgreen reviewed Aug 29, 2019

View reviewed changes

Pessimistress reviewed Sep 3, 2019

View reviewed changes

1chandu force-pushed the r/TransformRefactor branch from 3c93b83 to 5b2ec72 Compare September 7, 2019 23:13

1chandu changed the title ~~[POC] Transform refactor~~ Transform refactor Sep 7, 2019

1chandu commented Sep 7, 2019

View reviewed changes

Pessimistress reviewed Sep 9, 2019

View reviewed changes

Pessimistress approved these changes Sep 9, 2019

View reviewed changes

1chandu added 8 commits September 9, 2019 16:24

Transform refactor

fa907b8

Use seperate instances for Buffer and Texture transforms

42530dd

Replace old Transform class with new one

b77256a

Port #1224

eda3c9d

Merge buffer-transform-binding with buffer-transform

a08b1bb

Merge TextureTransformBinding with TextureTransform

c8f1a1b

Prefix private methods with _

f60b6d3

Add docs and, minor code cleanup

7b6cbb0

1chandu force-pushed the r/TransformRefactor branch from 5b2ec72 to 7b6cbb0 Compare September 9, 2019 23:32

1chandu merged commit 210151b into master Sep 9, 2019

1chandu deleted the r/TransformRefactor branch September 9, 2019 23:47

1chandu mentioned this pull request Sep 19, 2019

HeatmpaLayer: update required features for WebGL1 support. visgl/deck.gl#3656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform refactor #1221

Transform refactor #1221

1chandu commented Aug 28, 2019 •

edited

ibgreen left a comment

1chandu commented Aug 29, 2019 •

edited

Pessimistress Sep 3, 2019

Pessimistress Sep 3, 2019

1chandu commented Sep 7, 2019

1chandu Sep 7, 2019

Pessimistress Sep 9, 2019

1chandu Sep 9, 2019

Pessimistress Sep 9, 2019

Pessimistress Sep 9, 2019

coveralls commented Sep 9, 2019


		## Methods (Model props)

		### getDrawOptions(opts: Object) : Object

Transform refactor #1221

Transform refactor #1221

Conversation

1chandu commented Aug 28, 2019 • edited

Background

Change List

ibgreen left a comment

Choose a reason for hiding this comment

1chandu commented Aug 29, 2019 • edited

Pessimistress Sep 3, 2019

Choose a reason for hiding this comment

Pessimistress Sep 3, 2019

Choose a reason for hiding this comment

1chandu commented Sep 7, 2019

1chandu Sep 7, 2019

Choose a reason for hiding this comment

Pessimistress Sep 9, 2019

Choose a reason for hiding this comment

1chandu Sep 9, 2019

Choose a reason for hiding this comment

Pessimistress Sep 9, 2019

Choose a reason for hiding this comment

Pessimistress Sep 9, 2019

Choose a reason for hiding this comment

coveralls commented Sep 9, 2019

1chandu commented Aug 28, 2019 •

edited

1chandu commented Aug 29, 2019 •

edited