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.

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

Texture folder explicit #532

Merged
merged 6 commits into from
Jun 7, 2019
Merged

Conversation

elpie89
Copy link
Contributor

@elpie89 elpie89 commented Jun 4, 2019

users should be able to choose where to save textures,
GLTFspecification doesn't allow us to save outside on top of the model folder,
I display a warning in case the user does not respect this constraint
the same logic for the GLTF path (absolute/relative) is applied here.
I also made a mini improvement on the task kill function

a warnign message prevent user that texture cannot be exported with multiexport button
@Drigax
Copy link
Contributor

Drigax commented Jun 7, 2019

Looks like this works well. Thanks!

@Drigax Drigax merged commit 7bbeed6 into BabylonJS:master Jun 7, 2019
@Drigax
Copy link
Contributor

Drigax commented Jun 7, 2019

Forgot to mention, for future pull requests, try to have a more descriptive title and description, as we'll be pulling from the PRs to populate the Changelog.

@Drigax
Copy link
Contributor

Drigax commented Nov 14, 2019

GLTFspecification doesn't allow us to save outside on top of the model folder,
I display a warning in case the user does not respect this constraint

I took a look at the glTF image schema,

https://github.com/KhronosGroup/glTF/blob/d712689715e77500df405f557023225c68c655ae/specification/2.0/schema/image.schema.json#L12

Suggests that image URIs are arbitrary, I don't see any explicit restriction on only relatively pathing to images in a sub-path of the .gltf location. I assume this functionality is intended for loading web-hosted resources, but we aren't explicitly violating any schema in using arbitrary texture folder paths.
(it would only make manually loading glTF into a web viewer manually pretty much impossible)

@PatrickRyanMS what are your thoughts on this? especially given #685, do we want to make the UI give a warning to the user along the lines of "This is probably not a good idea"? Or do we want to impose this restriction for the sake of sanity?

@PatrickRyanMS
Copy link
Member

@Drigax, I had a long conversation with @bghgary about this to see if he could identify any reason not to allow saving textures to a directory other than one that is a child of the location of the glTF. He could not come up with a reason to disallow it, but did have a few good ideas about the flow.

  • One is to move the file format drop down to the top of the window so you set that first.
  • We should really change "Model Name" to "Model Path" or "Model Location" and mirror that with "Texture Path" or "Texture Location"
  • Once you input the model path, we should auto populate the texture path with the same path as the model. This will imply that you don't need to change it, but give you the opportunity to do so. It also does not incur any more clicks from the user as we would if we added a checkbox to specify texture path and does not change the UI layout dynamically.
  • If the user enters a texture path that is not a child of the location of the model, we should write a warning in the console window immediately that writing textures outside the path of the model may not work correctly for all render clients.
  • And if you are able to, he mentioned that the URIs should be canonicalized to make them as streamlined as possible.

@elpie89
Copy link
Contributor Author

elpie89 commented Nov 14, 2019

  • One is to move the file format drop down to the top of the window so you set that first.

Why not

  • We should really change "Model Name" to "Model Path" or "Model Location" and mirror that with "Texture Path" or "Texture Location"

I completely agree

  • Once you input the model path, we should auto populate the texture path with the same path as the model. This will imply that you don't need to change it, but give you the opportunity to do so. It also does not incur any more clicks from the user as we would if we added a checkbox to specify texture path and does not change the UI layout dynamically.

Yes

  • If the user enters a texture path that is not a child of the location of the model, we should write a warning in the console window immediately that writing textures outside the path of the model may not work correctly for all render clients.

I'm sure I made this some months ago if this is not happening it is a bug.

  • And if you are able to, he mentioned that the URIs should be canonicalized to make them as streamlined as possible.

I agree

This is definitely the most charged period of the year for me, I really would like to take a look on this next week in my free time, but I don't think I can take a look at this one before.

To give some context on the original problem, the texture path out of model folder feature has been made for non-web uses.
In a non-web-engine, it's vital to not duplicate textures shared between models

@PatrickRyanMS
Copy link
Member

If the user enters a texture path that is not a child of the location of the model, we should write a warning in the console window immediately that writing textures outside the path of the model may not work correctly for all render clients.

I'm sure I made this some months ago if this is not happening it is a bug.

I do see the pop-up warning about the texture path must follow the model path but this blocks you from using the specified path. As I was talking to @bghgary we came to the conclusion that with a relative path there may be a security issue from navigating above the current directory to get to another path, but there has been requests for just this feature from glTF users. To that end, we probably shouldn't block people from doing it, but just warn them in the console that if they make this decision it may not work everywhere.

@bghgary
Copy link
Contributor

bghgary commented Nov 14, 2019

For context, here are some discussions about URIs in glTF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants