Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(engine): Transform → BufferTransform + TextureTransform #1896

Merged
merged 10 commits into from
Jan 4, 2024

Conversation

donmccurdy
Copy link
Collaborator

Work in progress. Adding more functionality required for deckgl aggregation layers.

Changes:

  • Renames TransformBufferTransform
  • Adds TextureTransform class

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 21, 2023

My intentions here are:

  • BufferTransform: API-agnostic wrapper around Model which takes Model-like input props, and some additional output-related props, runs the Model, and writes the result of a vertex transformation (via Transform Feedback for now) to a Buffer.
  • TextureTransform: API-agnostic wrapper around Model which takes Model-like input props, and some additional output-related props, runs the model, and writes output of the fragment shader to a texture.

In comparison to the current implementation I'm hoping to avoid all of the shader transformation happening in code like transform-shader-utils.ts, with the expectation that callers of the Transform classes do a bit more work to set up a valid Model and shaders. Simple passthrough fragment shaders can still be autogenerated.

I'm hoping this can considerably simplify the code in preparation for API changes in v9.1. I realize there may be some wrinkles in this plan. I did for example see code in deckgl running transform feedback for transitions and also writing output to a 1-pixel fragment shader to detect when a transition has completed. That can probably be handled here but if there are other complicating factors it would be good to know!

I'd also like to try to ensure that the important functionality of the Transform classes is verified directly with unit tests, which will make future changes much easier. This PR contains a simple 'mean' aggregation, which is not working yet. Probably either I'm not drawing pixels to the right coordinates of the framebuffer, or I haven't bound the framebuffer correctly. Currently after transform.run() only the clear color comes back.

@donmccurdy donmccurdy force-pushed the donmccurdy/texture-transform branch 3 times, most recently from d263a9f to 06f719f Compare December 21, 2023 22:01
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 21, 2023

To do:

  • fix test failures
  • add a test with texture as input
  • add a test with 'swap' behavior
  • clean up TODOs

@donmccurdy donmccurdy force-pushed the donmccurdy/texture-transform branch from 06f719f to 7bf0630 Compare December 27, 2023 16:50
@donmccurdy donmccurdy marked this pull request as ready for review December 28, 2023 18:54
@donmccurdy
Copy link
Collaborator Author

This should be ready for review. I think the remaining TODOs in code represent open questions, related to my comment above. Thoughts or preferences would be welcome. In the meantime I'll do some tests locally updating deckgl against this PR, to see if I have further opinions on the TODOs.

This is a 'breaking' change to the previous transform API and is, hopefully:

  1. sufficient to cover deckgl's current needs
  2. easier to understand and update

@donmccurdy donmccurdy changed the title Draft: Add TextureTransform, rename Transform to BufferTransform [engine] Transform → BufferTransform + TextureTransform Dec 28, 2023
@donmccurdy donmccurdy changed the title [engine] Transform → BufferTransform + TextureTransform feat(engine): Transform → BufferTransform + TextureTransform Dec 28, 2023
@donmccurdy donmccurdy force-pushed the donmccurdy/texture-transform branch from 5357995 to 2284843 Compare December 29, 2023 15:01
@donmccurdy donmccurdy changed the base branch from master to donmccurdy/assemble-glsl-explicit-version December 29, 2023 15:30
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 29, 2023

modules/engine/src/transform/texture-transform.ts Outdated Show resolved Hide resolved
modules/engine/src/transform/texture-transform.ts Outdated Show resolved Hide resolved
modules/engine/src/transform/texture-transform.ts Outdated Show resolved Hide resolved
modules/engine/test/transform/texture-transform.spec.ts Outdated Show resolved Hide resolved
modules/engine/test/transform/texture-transform.spec.ts Outdated Show resolved Hide resolved
modules/engine/test/transform/texture-transform.spec.ts Outdated Show resolved Hide resolved
modules/engine/test/transform/texture-transform.spec.ts Outdated Show resolved Hide resolved
modules/engine/test/transform/texture-transform.spec.ts Outdated Show resolved Hide resolved
Base automatically changed from donmccurdy/assemble-glsl-explicit-version to master January 2, 2024 14:42
@donmccurdy donmccurdy force-pushed the donmccurdy/texture-transform branch from 5be0b8b to 958fe33 Compare January 2, 2024 19:03
@donmccurdy donmccurdy force-pushed the donmccurdy/texture-transform branch from 958fe33 to 52d9794 Compare January 2, 2024 19:26
donmccurdy and others added 2 commits January 2, 2024 14:28
Co-authored-by: felixpalmer <felixpalmer@gmail.com>
@donmccurdy donmccurdy merged commit beba1c0 into master Jan 4, 2024
1 of 2 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/texture-transform branch January 4, 2024 14:40
@donmccurdy donmccurdy added this to the 9.0.0 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants