-
Notifications
You must be signed in to change notification settings - Fork 56
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
Aeroglass features #153
base: purgatory
Are you sure you want to change the base?
Aeroglass features #153
Conversation
…ob3mobile/g3m into aeroglass-collision-avoidance
…ob3mobile/g3m into aeroglass-collision-avoidance
…gger data files by saving them separately and saving a pointer into the database
…rms from a gl feature
…d way before this
…custom shaders and used instead
Hi @DrakkLord This PR is a bit too big. There features that we think can be merged, but mixed with other changes we need to discuss before merging. Specifically I don't think I like the changes on the shaders mechanism, specially the GPUAttributeKey.UNRECOGNIZED_ATTRIBUTE and related stuff. One of the goals in the shaders subsystem is that the attributes/uniforms have to much 1to1 between the C++ code and the shaders, it means: there must be NO unrecognized things at all. Also I'd like to hear what @amazingsmash think about this. |
The shader system is made like this to avoid what you described as making custom shaders requires changing G3M source code which one might not want to do. Sorry about the big PR this happened on our end we won't make such big PR's in the future. |
In general, I agree with @DiegoGomezDeck . The system should allow you to select any specific shader based on a unique set of entries. One of the main purposes of the whole shader mechanism is to ensure the consistency between the shader used and the provided information. Taking a look at the PR the GPUAttributeKey.UNRECOGNIZED_ATTRIBUTE seems to break that feature. On the other hand, I agree with @DrakkLord that the system, at this moment, does not allow new shaders to be used. IG3MBuilder lacks a method to include new shaders, and the new variables of those shaders should be recognized properly via GPUVariable. |
Thanks for your input @amazingsmash, we've seen this was the goal of the GPUKeying however this creates a closed system where you must make new keys and most importantly you have to modify the shader selection because they are based on keys so for example if I want to do two different visual styles that use the same keys I can't do that because based on keys one shader would override the other same goes for completely new shaders. However I agree that this could be done better, the main focus with this feature was to make G3M allow us to use shaders more flexibly so essentially if you introduce a new mechanism to handle say "unknown attribute for G3M" you would do the same as it is right now since G3M does nothing with unrecognized variables, and like I mentioned on top of this, the shader selection is hard-wired which disallows making any custom visuals. @DiegoGomezDeck seeing that the focus is on the custom shader feature, should we extract this feature move it to another branch and discuss how it can be implemented and then remaining things in this branch can be merged? |
Hi @DrakkLord Let's split this PR into different branches. The features not related to the shaders mechanism looks good to me, and I think we can merge them soon. Related to the other important part, let cal lit "custom shaders"... let's open an issue/PR and move the discussion there. We agree with you we need a way to add custom-shaders, let's find out the way. |
Hi @DiegoGomezDeck !
We decided to not make pull requests on the aeroglass branch, instead we will make pull requests from individual feature branches, so here is branch with out current feature changes.