Skip to content
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.

Sign up for GitHub

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

Jump to bottom

image uri fix #688

Merged
merged 3 commits into from
Nov 26, 2019
Merged

image uri fix #688

merged 3 commits into from
Nov 26, 2019

Conversation

elpie89
Copy link
Contributor

@elpie89 elpie89 commented Nov 21, 2019 •
edited
Loading

This should be ok
I also changed the UI as requested
#685

@Drigax
Copy link
Contributor

Drigax commented Nov 22, 2019

Thanks for the fix, I'll take a look at this first thing tomorrow!

@Drigax
Copy link
Contributor

Drigax commented Nov 22, 2019

Alright, looks good for the most part, I'd just like to add a warning to our output window when we are placing textures in a directory that's not in a subdirectory of the glTF

Drigax
Drigax suggested changes Nov 22, 2019
View reviewed changes
3ds Max/Max2Babylon/Forms/ExporterForm.cs Outdated Show resolved Hide resolved
Drigax
Drigax reviewed Nov 22, 2019
View reviewed changes
3ds Max/Max2Babylon/Forms/ExporterForm.cs Outdated

if (!PathUtilities.IsBelowPath(selectedFolderPath, absoluteModelPath))
{
MessageBox.Show("WARNING: folderPath should be below model file path");
CreateWarningMessage("WARNING: textures path should be below model file path, not all client renderer support this feature",0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: can you change "client renderer" to "client renderers"?

Drigax
Drigax reviewed Nov 22, 2019
View reviewed changes
SharedProjects/Babylon2GLTF/GLTFExporter.Texture.cs Outdated
}
else
{
textureUri = Path.Combine(textureUri, name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On windows, we end up having backslashes in our image URI path, after getting this relative path. should we replace that with forward slashes?

SharedProjects/Babylon2GLTF/GLTFExporter.cs
@@ -482,7 +482,7 @@ private List<GLTFBufferView> SwitchImagesFromUriToBinary(GLTF gltf)

foreach (GLTFImage gltfImage in gltf.ImagesList)
{
var path = Path.Combine(gltf.OutputFolder, Uri.UnescapeDataString(gltfImage.uri));
var path = Path.Combine(gltf.OutputFolder, gltfImage.uri);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we no longer escaping the symbols in our path?

Copy link
Contributor Author

@elpie89 elpie89 Nov 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is already unescaped in Path.GetRelativePath..this was a repetition.
the gltfimage uri is set only in L176

@elpie89
Copy link
Contributor Author

elpie89 commented Nov 23, 2019 •
edited
Loading

Everything is now managed in GetRelativePath() method
the relative part is calculated using URI and I removed the part where forward slashes were transformed in backslashes
the Path.Combine() does not alter the slash..so everything is now forwarded.
I think we should stay with URI format, as windows accept both.

Also, I do not remove the first "./"
It is useless and the URI format accept it

the second warning has been added too, in case a model path is set after the textures one

we end up with this 3 cases:

"uri": "../../texture/checker.jpg" (with warning raised)
"uri": "./checker.jpg"
"uri": "texture/checker.jpg"

If I did not forget some case, should be ok

Drigax
Drigax reviewed Nov 26, 2019
View reviewed changes
SharedProjects/Babylon2GLTF/GLTFExporter.cs
@@ -470,7 +470,11 @@ private string gltfToJson(GLTF gltf)
// Use the bounded writer in case some values are infinity ()
using (var jsonWriter = new JsonTextWriterBounded(sw))
{
#if DEBUG
Copy link
Contributor

@Drigax Drigax Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition!

Drigax
Drigax approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@Drigax Drigax left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! This did expose some issue with our model loader, but the current location designator doesn't violate the glTF uri schema. I filed BabylonJS/Babylon.js#7210 to address this properly.

I don't think this is an issue in your formatting.

@Drigax Drigax merged commit d8dfd95 into BabylonJS:master Nov 26, 2019
@elpie89 elpie89 deleted the image_uri_fix branch January 28, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Drigax Drigax Drigax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elpie89 @Drigax