search

glob3mobile / g3m

The multiplatform advanced visualization framework
http://www.glob3mobile.com/
Other
116 stars 56 forks source link

Aeroglass features #153

Open DrakkLord opened 8 years ago

DrakkLord commented 8 years ago

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.

DiegoGomezDeck commented 8 years ago

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.

DrakkLord commented 8 years ago

Hi @DiegoGomezDeck

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. For example if I would like to just use G3M and not modify its source then I cannot use custom graphics via shaders only the ones built in. With this modification G3M allows the custom stuff by adding your own shader source into the widget builder and using it from your own GL feature and by doing this you are responsible handling the shader attributes / uniforms that G3M flags as UNRECOGNIZED.

Sorry about the big PR this happened on our end we won't make such big PR's in the future.

amazingsmash commented 8 years ago

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.

DrakkLord commented 8 years ago

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?

DiegoGomezDeck commented 8 years ago

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.