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: Refactor shader uniform binding to support shader arrays [flame_3d] #3282

Merged
merged 2 commits into from
Sep 21, 2024

Conversation

luanpotter
Copy link
Member

@luanpotter luanpotter commented Aug 25, 2024

Description

Refactor shader uniform binding to support shader arrays.

This also decouples the whole shader and uniform byte handling code (that we should definitely test) from the flutter_gpu primitives that are impossible to mock (base native classes).

This adds tests that ensure the arrays are bound as they should - however the underlying flutter_gpu code does not seem to work. See this PR for a test of using this to support an arbitrary number of lights.

Either way, we can merge this as is as this refactors the underlying structure to support arrays when ready, and make it more testable as well.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

@luanpotter luanpotter changed the title feat: Refactor shader uniform binding to support shader arrays feat: Refactor shader uniform binding to support shader arrays [flame_3d] Sep 11, 2024
@luanpotter luanpotter marked this pull request as ready for review September 12, 2024 21:01
@@ -13,7 +13,7 @@ class ResizingRectangle extends RectangleComponent {
void onGameResize(Vector2 size) {
super.onGameResize(size);

this.size = size * .4;
this.size = size * 0.4;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated DCM

_vertexShader.resource,
_fragmentShader.resource,
_vertexShader.compile().resource,
_fragmentShader.compile().resource,
Copy link
Member Author

@luanpotter luanpotter Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to any other cleaner ways to make this testable

@spydon
Copy link
Member

spydon commented Sep 12, 2024

Have you reported that it doesn't work in flutter_gpu?

@luanpotter
Copy link
Member Author

I've been talking with @bdero about it; he suggested using DeviceBuffer instead of HostBuffer but I haven't had time to try it yet.

Either way, I believe the Flame-facing API should be independent of this (though I def want to completely overhaul it to be type safe, the current one is reliant on strings).

luanpotter added a commit that referenced this pull request Sep 13, 2024
Support skeletal animation basics.

Sadly we cannot support arrays yet - see [this
PR](#3282).
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@luanpotter luanpotter merged commit 0c0e8f2 into flame_3d Sep 21, 2024
7 checks passed
@luanpotter luanpotter deleted the luan.shader-arrays branch September 21, 2024 00:18
luanpotter added a commit that referenced this pull request Oct 15, 2024
Support skeletal animation basics.

Sadly we cannot support arrays yet - see [this
PR](#3282).
luanpotter added a commit that referenced this pull request Oct 15, 2024
…_3d] (#3282)

Refactor shader uniform binding to support shader arrays.

This also decouples the whole shader and uniform byte handling code
(that we should definitely test) from the flutter_gpu primitives that
are impossible to mock (base native classes).

This adds tests that ensure the arrays are bound as they should -
however the underlying flutter_gpu code does not seem to work. See [this
PR](#3284) for a test of using
this to support an arbitrary number of lights.

Either way, we can merge this as is as this refactors the underlying
structure to support arrays when ready, and make it more testable as
well.
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