That commit was part of pull request 2086. Here’s a branch visualization using the GitK tool, with commit e440f31 tagged:
@codex, would you please take a look at the test model and tell us what’s going on?
4 Likes
This PR appears to be setting “UseVertexColors” as true as long as a VertexColor Buffer exist:
However this is now turning the setting on for PBR models that do have a color buffer, but are NOT intended to use vertex colors.
There are a handful of reasons that a model may have a vertex Color Buffer without intending to have UseVertexColors set true:
-
During the modeling/texturing process, its common for the artist to use vertex color for Color ID groups, and many texturing programs recommend this (or a similar) workflow. This results in many PBR models that will have a vertex buffer but do not use them as vertex colors in the final render.
-
Some special shaders rely on vertex colors for other things. For example, the PBRTerrainShader (and many of my own forked PBR shaders) use the red channel of the vertex color buffer as a DirectionalLight occlusion value. Some other shaders I have seen will use the vertex buffer for simulating windiness intensity on a tree/shrub model.
-
Some artists simply just use the vertex colors as a rough-draft to help pick colors and visualize the final result before moving onto the detailed texturing proces.
So (unless I am mistaken) I think that this PR needs to be reverted, or changed so that it uses a different indicator to determine if UseVertexColors should be automatically enabled.
3 Likes
Thanks for your quick analysis, @yaRnMcDonuts .
I hope we can come up with a better algorithm for enabling UseVertexColors.
2 Likes
I think it might be an easy thing to check by setting a material parameter override on the root of the loaded spatial that turns off UseVertexColors. Could be there are also other things in play.
Unfortunately, gltf doesn’t have such an indicator that I am aware of, so we can’t really differentiate between the use cases purely from the gltf file. On the other hand, reverting the PR will make JME not directly meet the gltf specification for vertex colors.
3 Likes
If the goal is for JME to completely match their specifications then you are right (based on what I see here: glTF™ 2.0 Specification)
But that means will also have more future issues like this anytime someone tries converting a model that was not created by an artist who was strictly adhering to their standards in regards to vertex colors.
So prior to your PR, there technically could have been unknowingly “broken” models that should have been using the vertex colors as an additional linear color multiplier. (although I’ve never seen any models that do this in combination with standard material colors/textures, it seems like a useless extra step if you already are using material colors, so I am surprised the gltf2.0 standards are still worried about supporting vertex colors in combination with material colors/textures like this)
In regards to vertex colors, they state: Client implementations **SHOULD** support at least two texture coordinate sets, one vertex color
and also
In addition to the material properties, if a primitive specifies a vertex color using the attribute semantic property COLOR_0, then this value acts as an additional linear multiplier to base color.
So if we do want to follow their specifications in this case then your PR is correct, but I’ve seen more models that use the vertexColor Buffer as something else than a linear color multiplier, so I’m surprised the gltf specifications are specific about that being the default, rather than making it based off of a togglable boolean (like it is in Jmonkey)
3 Likes
We could add a boolean to GltfModelKey to toggle between the glTF specification and ignoring vertex colors (as in JME 3.6.x).
6 Likes
Spatial model = ...load the model...
model.addMatParamOverride(new MatParamOverride(VarType.Boolean, "UseVertexColors", false));
Does it fix the problem for the model in question?
1 Like
Yes, it handles this, I tried it, but the parameter is: “UseVertexColor” No “s”
in addition:
I imported the models into several tools (including re-importing into Blender) and they looked like this:
(Never mind the transparency issues in Substance)
In 3DViewer
In Substance
Re-importing into Blender.
Maybe our new code is correct and conforms to the glTF2.0 specification, but it would be better if we could handle these issues more intelligently.
Maybe there are many legacy models that do not adhere to the specification.
3 Likes
I would go for what is correct. Especially if it really looks like we are 100% correct on this as per specs and the way others are doing this also. Trying to fix broken models by guessing is endless swamp, taking one guess breaks the other guesses etc.
3 Likes
Yeah, especially when the fix is one line of code that could be added to a JMEC script.
3 Likes
OK Guys, I would like to summarize the problems we have so far in V3.7:
- With the new glTF/GLB model loader, the AnimComposer and SkinningControl controllers are duplicated for each Geometry
- Blured colors - maybe normal map is missing?
- Sometime missing animations - maybe related to 1
- Sometime missing colors - maybe can be fixed with :
mySpatial.addMatParamOverride(new MatParamOverride(VarType.Boolean, "UseVertexColor", false));
In my test case it didn’t work…
5. The GLB loader encapsulates the a Plane geometry within two parent nodes - may introduce backward compatibility issue which needs to be documented.
I hope I got it right. Please correct me if I’m wrong.
@sgold , Should I open an issue in GitHub for each of these topics?
10X
5 Likes
Should I open an issue in GitHub for each of these topics?
That’s for you, as the release manager, to decide. Personally, I prefer to document issues at GitHub rather than tracking them in Discourse or a Google Doc.
Furthermore, I find that documenting issues in full detail facilitates both collaboration and clear thinking.
Edit: Sometimes all you have to do is ask the reporter to file an issue at GitHub, and they will do it for you!
3 Likes
Done. 
1 Like
@adi.barda is my PR (already merged into master) low risk enough to cherry pick into 3.7? It is just new asserts to help debug new asserts introduced in 3.7
2 Likes
I’m on it 
2 Likes
I’ve traced one of the symptoms of issue 2277 to PR 2060, which introduced an extra level of abstraction into JSON parsing.
GltfLoader relies heavily on JSON parsing. However, it’s not clear to me how this change broke loading of the YBot model. @RiccardoBlb are you available to investigate?
2 Likes
@sgold , while in the bisect process & locally compile a given state, how do you apply the changes to the test app? are you using a “lib” folder to load the compiled version and disable the remote repo or using some kind of local repo - something like this:
repositories {
// Add your local repository path here
maven {
url uri('path/to/your/local/repo')
}
// You can still keep other repositories like Maven Central or JCenter
mavenCentral()
}
if local repo is used then what is the way to register / install the files to that repo? do we have a JME command line to create a local repo or should we use something like :
mvn install:install
?
1 Like
Like gradle install?
2 Likes
There’s more than one way to do it. In this instance, I added the two test files directly to my clone of the “jmonkeyengine” repository:
jme3-examples/src/main/java/jme3test/model/TestYBot.java
jme3-examples/src/main/resources/YBot.gltf
That speeds things up and eliminates possible SNAFUs due to forgetting to update a Maven repo.
As Paul indicated, our buildscripts define an install task to install all JME libraries to the local Maven repository:
1 Like