-
Notifications
You must be signed in to change notification settings - Fork 472
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
Support for vrm. #199
Support for vrm. #199
Conversation
9c101cd
to
a5368cd
Compare
How do I read those errors in the cicd? Edited: What are the step to recreate the error? |
@zeux I wasn't able to recreate the errors. Do you know how that test works? |
The test validates that packing any model from glTF-Sample-Models repository produces a valid glTF file as validated by glTF-Validator. glTF-Sample-Models/2.0/Cameras/glTF/Cameras.gltf is the specific file on which the process fails with this change, you should be able to get the source file from glTF-Sample-Models Khronos repository. |
83132ed
to
c24d873
Compare
Green! |
Use a vrm file extension to trigger processing.
@@ -165,6 +175,8 @@ static void parseMeshesGltf(cgltf_data* data, std::vector<Mesh>& meshes, std::ve | |||
meshes.push_back(Mesh()); | |||
Mesh& result = meshes.back(); | |||
|
|||
result.name = mesh.name; |
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.
Might be a problem with variable lifetime
@@ -488,6 +498,11 @@ static void process(cgltf_data* data, const char* input_path, const char* output | |||
append(json_meshes, ",\"indices\":"); | |||
append(json_meshes, index_accr); | |||
} | |||
if (settings.keep_extras && !prim.extras.empty()) |
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.
Why is this necessary? AFAICT the only "extras" node in the .VRM file from the issue is on the mesh, not on primitive, and that contains targetNames which should already be preserved by gltfpack.
for (size_t i = 0; i < data->data_extensions_count; ++i) | ||
{ | ||
cgltf_extension extension = data->data_extensions[i]; | ||
if (!extension.name) |
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.
This & above & below can be replaced with if (strcmp(extension.name, "VRM") == 0)
(extension.name is guaranteed to be non-null)
@@ -676,6 +722,8 @@ static void process(cgltf_data* data, const char* input_path, const char* output | |||
writeArray(json, "meshes", json_meshes); | |||
writeArray(json, "skins", json_skins); | |||
writeArray(json, "animations", json_animations); | |||
comma(json); | |||
append(json, json_extensions); |
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.
This will always emit "extensions" and also causes a conflict with top-level KHR_lights_punctual extension since there's now going to be two extension blocks for files that use that.
Instead what should happen is:
json_extensions
should be initially emptyjson_lights
should be appended to it, if necessary- VRM data should be appended to it, if necessary
- if
json_extensions
isn't empty when forming the final JSON blob, we should write out"extensions":{
followed by json_extensions contents followed by}
append(json_extensions, "\""); | ||
append(json_extensions, extension.name); | ||
append(json_extensions, "\":"); | ||
append(json_extensions, extension.data); |
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.
This should use appendJson
to minify the output
gltf/gltfpack.cpp
Outdated
@@ -790,8 +838,12 @@ int gltfpack(const char* input, const char* output, const char* report, Settings | |||
|
|||
const char* iext = strrchr(input, '.'); | |||
const char* oext = output ? strrchr(output, '.') : NULL; | |||
|
|||
if (iext && (strcmp(iext, ".vrm") == 0 || strcmp(iext, ".VRM") == 0)) { |
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 use clang-format on this code, as this bracing style is inconsistent with the coding guidelines, see https://github.com/zeux/meshoptimizer/blob/master/CONTRIBUTING.md
if (oext && (strcmp(oext, ".vrm") == 0 || strcmp(oext, ".VRM") == 0)) | ||
{ | ||
settings.vrm = true; | ||
settings.keep_extras = true; |
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 would prefer to omit these, esp. quantize
but preferably others. It's not clear to me why these need to be present, and if a user wants to apply these optimizations to a VRM file there's not going to be a way to do this without adding more kinds of command-line options, if vrm
extension acts as an override.
@@ -896,7 +958,7 @@ int gltfpack(const char* input, const char* output, const char* report, Settings | |||
return 4; | |||
} | |||
} | |||
else if (oext && (strcmp(oext, ".glb") == 0 || strcmp(oext, ".GLB") == 0)) | |||
else if (oext && (strcmp(oext, ".vrm") == 0 || strcmp(oext, ".VRM") == 0 || strcmp(oext, ".glb") == 0 || strcmp(oext, ".GLB") == 0)) |
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.
Should this also be added to the condition above that sets texture_embed
? Unclear what the de-facto rules are for VRM files, e.g. whether they are supposed to keep textures separately or not.
gltf/gltfpack.h
Outdated
@@ -39,6 +39,7 @@ struct Transform | |||
|
|||
struct Mesh | |||
{ | |||
const char* name; |
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.
std::string
might be better.
gltf/write.cpp
Outdated
@@ -739,6 +739,14 @@ static std::string decodeUri(const char* uri) | |||
|
|||
void writeImage(std::string& json, std::vector<BufferView>& views, const cgltf_image& image, const ImageInfo& info, size_t index, const char* input_path, const char* output_path, const Settings& settings) | |||
{ | |||
if (image.name) |
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.
Is preserving the name of the image object necessary? It doesn't carry semantical meaning, and gltfpack right now only preserves names for objects that are likely to be used by-name externally, specifically nodes, materials and animations.
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 need to check if this is required by the internal material definition called MTOON.
gltf/write.cpp
Outdated
@@ -1320,7 +1328,8 @@ void writeAnimation(std::string& json, std::vector<BufferView>& views, std::stri | |||
|
|||
comma(json_samplers); | |||
append(json_samplers, "{\"input\":"); | |||
append(json_samplers, range ? range_accr : track.constant ? pose_accr : time_accr); | |||
append(json_samplers, range ? range_accr : track.constant ? pose_accr |
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.
Was this change intended?
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.
Not intended.
Thank you for the review. I will be delayed until the weekend to submit the changes. |
I slept thinking about this optimization, because Godot Engine added autolod, I don't have a requirement for this. This VRM pr is salvageable. |
still interessded in vrm optimation @fire can you point me to how Godot Engine autolod works? |
I believe that mesh optimizer works under wasm, so you can follow the same flow. The reason for this flow is that it doesn't make sense to modify the original gltf if the goal is to convert to the internal engine format. |
Use a vrm file extension to trigger processing.
See #198 for test cases and examples.
The command to test is now:
./gltfpack -i "Miraikomachi.vrm" -o "Miraikomachi Opt.VRM"