-
Notifications
You must be signed in to change notification settings - Fork 347
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
Add global shader list support to s2d #1265
base: master
Are you sure you want to change the base?
Add global shader list support to s2d #1265
Conversation
@@ -781,7 +830,7 @@ class RenderContext extends h3d.impl.RenderContext { | |||
flush(); | |||
var shaderChanged = needInitShaders, paramsChanged = false; | |||
var objShaders = obj.shaders; | |||
var curShaders = currentShaders.next; | |||
var curShaders = currentShadersTail.next; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think this needs a null check
var curShaders = currentShadersTail.next; | |
var curShaders = currentShadersTail?.next; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because it's never supposed to be null. Tail points at the last global shader (tail.next
points at object-specific shaders), and you should not be able to remove base shader from the global shader list, thus tail is never null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 idk i had to change this to make it work locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an repro sample?
Shouldn't we have instead something like push/pop mechanism so we can use it during rendering ? |
Elaborate? This is pretty much how it currently behaves. Nothing prevents user from adding shaders mid-rendering and then removing them. Or you mean somehting along the lines |
Introduces ability to add global shaders to 2D rendering pipeline.
Rationale: Allows users to add shaders to multiple rendered objects without having to add same shader to all of them, or apply a shader that have to be always present on all 2D objects. This reduces the amount of tedious shader management.
Use-case examples:
Adds the following API:
RenderContext.addGlobalShader(shader)
RenderContext.removeGlobalShader(shader)
A few notes:
currentShaders
to even exist? It seems fairly redundant, asinitShaders
only ever called withbaseShaderList
, and thus it can never be anything other thanbaseShaderList
. Some legacy code leftovers?Cache.compileRuntimeShader
baseShaderList
toglobalShaderList
.