-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Shader system improvements and fixes. #2577
Shader system improvements and fixes. #2577
Conversation
Implemented using WebAudio API targeting HTML5 and OpenAL targeting Windows & Neko
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.
PLEASE MERGE THIS!!!
Interesting. I was looking for a feature to change a CONST value at runtime. I think storing fragment body and header would be easier for me than parsing, diassembling an reassembling the whole shader. Some shaders need to use CONST in a for loop because non-constant values are forbidden, but different values can provide a better accuracy depending on what has been set in the uniform (e.g: Glow, Outline...). So it's looking great. That being said, if you have only tested on Windows. This need to be tested in HL, MAC, Android, iOS and HTML5 before being merged. Also can you provide details on your issue on "some platforms", which platforms are your talking about? |
i tested it on mac and i can confirm it works on mac too |
@mastereric This pull request makes android to crash, I think is because of |
what glversion should be put to use for android? |
Idk the exact version |
You have to find out, is your pull request anyway |
the glsl version for android is #version 300 es how i found out is because of the flxFixedShader https://github.com/YoshiCrafter29/YoshiCrafterEngine/blob/45ddea338a211ff2d05b7d85a3a2c38bb8ab287f/source/FlxFixedShader.hx#L55 |
That version is wrong |
…ssion' into develop
Took a bit of time today to fast forward the branch, as well as implement a couple fixes. Mainly, Hopefully this works for people, I've been having reports that AMD Radeon cards are having issues specifically. |
Dude, the default version by openfl is 100 |
If you put 300 es it will broke everything |
This PR is now ready and fully tested on a variety of platforms. See more at my test workbench here: https://github.com/MasterEric/Flixel-GalaxyShaderWorkbench EDIT: Make sure to use the build instructions to ensure the correct library versions are installed before testing. |
With help from @JonnycatMeow I was able to test the workbench on their Mac platform. Notably, their version of OpenGL ONLY supports |
bro can someone merge this please!!!! |
The Sound modification seems pretty handy for streaming |
This pull request is not related to sound. |
Look on the Commits tab of this PR. You merged in a |
Thank you for informing me of this. I will re-review my branch, make appropriate fixes, and merge them ASAP so that review can continue. |
It appears that changes related to other pull requests (#2534 and #2515) accidentally got included in the working branch for this PR. Those changes have now been removed, and the PR has additionally been fast-forwarded to match the target branch. Please inform me here of any additional review changes. |
default: | ||
return glExtensions; | ||
|
||
case "100": | ||
return glExtensions; | ||
case "110": | ||
return glExtensions; | ||
case "120": | ||
return glExtensions; | ||
case "130": | ||
return glExtensions; | ||
case "140": | ||
return glExtensions; | ||
case "150": | ||
return glExtensions; | ||
|
||
case "300 es": | ||
var hasSeparateShaderObjects = false; | ||
for (extension in glExtensions) | ||
{ | ||
if (extension.name == "GL_ARB_separate_shader_objects") hasSeparateShaderObjects = true; | ||
if (extension.name == "GL_EXT_separate_shader_objects") hasSeparateShaderObjects = true; | ||
} | ||
|
||
#if desktop | ||
if (!hasSeparateShaderObjects) | ||
{ | ||
#if linux | ||
return glExtensions.concat([{name: "GL_EXT_separate_shader_objects", behavior: "require"}]); | ||
#else | ||
return glExtensions.concat([{name: "GL_ARB_separate_shader_objects", behavior: "require"}]); | ||
#end | ||
} | ||
#end | ||
return glExtensions; | ||
|
||
case "310 es": | ||
var result = buildGLSLExtensions(glExtensions, "300 es", isFragment); | ||
return result; | ||
|
||
case "320 es": | ||
var result = buildGLSLExtensions(glExtensions, "310 es", isFragment); | ||
return result; | ||
|
||
case "330": | ||
var result = buildGLSLExtensions(glExtensions, "320 es", isFragment); | ||
return result; | ||
|
||
case "400": | ||
return glExtensions; | ||
case "410": | ||
return glExtensions; | ||
case "420": | ||
return glExtensions; | ||
case "430": | ||
return glExtensions; | ||
case "440": | ||
return glExtensions; | ||
case "450": | ||
return glExtensions; | ||
case "460": | ||
return glExtensions; |
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.
I don't get why this switch seems so needlessly convoluted.
- Why have a bunch of cases that behave exactly like the default case?
- why not group similar results using
case "400" | "410":
? - why "310 es" pass "300 es" into
buildGLSLExtensions
and so on for "320 es" and "330"? - why store a result var just to return it
// We don't know this GLSL version, so don't do anything. | ||
return source; |
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.
Maybe show an explicit warning like "OpenFl doesn't know glsl version ${glVersion}, no conversion will happen" ?
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.
So then how do we use a certain glsl version in the project.xml or could it only be used in .hx code?
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.
You don't specify GLSL version in the project.xml you specify it when you construct the shader
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.
oh ok so then the default whould be 120 for desktops and for web it whould be 300?
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.
oh ok so then the default whould be 120 for desktops and for web it whould be 300?
The default would be the result of getDefaultGLSLVersion
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.
ohhh ok
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.
then how whould i use a certain glsl version?
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.
then how whould i use a certain glsl version?
You can use @:glVersion("120")
the same way you use @:glFragmentBody("...")
.
As a note there is an FlxRuntimeShader
in flixel-addons
. It was originally built by me and ran on this, but it was modified to not use this code so it could be merged. When this is merged it will get changed back and then it will support setting the glVersion
among other things.
When did i said this what |
I accidentally included another PR in this one by mistake. |
there should be core and compatibility support for macOS |
Closed in favor of #2715. |
Provides the following changes:
glVersion
annotation (and corresponding variables) which can be used to modify the GLSL version directive. The default value has been set to120
, which should resolve an issue on some platforms where theuniform
type is unavailable.__processGLData
to ensure a variable exists before it is assigned via Reflection.Reflect.fields
, improving performance.These changes were performed to support fixes for FlxRuntimeShader, as part of the pull request HaxeFlixel/flixel-addons#368. Making changes here prevents code duplication in FlxRuntimeShader, improving maintainability.
These changes have been tested on the Windows HXCPP platform.