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

Make glTF feature #156

Closed
kvark opened this issue Jan 3, 2018 · 10 comments
Closed

Make glTF feature #156

kvark opened this issue Jan 3, 2018 · 10 comments

Comments

@kvark
Copy link
Collaborator

kvark commented Jan 3, 2018

I noticed that we are spending multiple seconds compiling even some trivial changes. We need to investigate and see if something can be done about this. E.g. we could hide gltf stuff behind a (default) feature, or some derivatives, or use less generics, etc.

@alteous
Copy link
Member

alteous commented Jan 3, 2018

I feel I must apologise for the atrocious compile time of gltf. It's become a bit of a joke. 😛

@kvark
Copy link
Collaborator Author

kvark commented Jan 3, 2018

@alteous would you be good with having gltf feature? I think it doesn't even have to be default.

@alteous
Copy link
Member

alteous commented Jan 3, 2018

Yes, that's perfectly reasonable. Be aware though the documentation uses gltf in the examples.

@kvark
Copy link
Collaborator Author

kvark commented Jan 3, 2018

@alteous not sure, but perhaps the animation.rs documentation should provide non-gltf examples instead

@alteous
Copy link
Member

alteous commented Jan 3, 2018

Although the compile time of gltf is really awful, it would be a shame to not have it as a default feature since it would limit the available 3D formats to just OBJ. Also, although glTF standardises PBR, we don't need to have the PBR shader with the glTF loader since in many cases we can fall back to the Basic material kind.

@kvark
Copy link
Collaborator Author

kvark commented Jan 3, 2018

I don't think having it as a feature would exclude anyone. Adding a simple features = ["gltf"] to Cargo seems simple enough. I see it a candidate for a feature because it's fairly self-contained and has it's own specific external dependencies and examples.

@alteous
Copy link
Member

alteous commented Jan 3, 2018

I agree it should be a feature; I'm saying it should be enabled by default.

@kvark
Copy link
Collaborator Author

kvark commented Jan 4, 2018

Let me clarify my position, please:

  1. By no means I want to make the current glTF logic unavailable to the users - it's amazing, and I consider it a pillar of three-rs. Not that many projects feature high quality glTF + PBR + animation support.
  2. We both seem to agree that gltf needs to be gated a feature
  3. If the question raises "should this feature be default", then given all our equals (!), the answer should be "no". Users who don't care will get faster compile times (and run-times, given no need to link some shaders?). Users who do care - can easily switch it on. Unless there is some negative effect on the core function of the project, a feature shouldn't be default.
  4. Same can be said about obj, but it's way less code there.

@alteous
Copy link
Member

alteous commented Jan 5, 2018

Reluctantly, I'm happy to let gltf be opt-in for the sake of compile times. I hesitate due to conflict of interest of wanting to see glTF as the 'standard' ("jpeg of 3D") runtime format for 3D opposed to other legacy and, frankly, inferior formats.

bors bot added a commit that referenced this issue Jan 5, 2018
160: Add gltf-loader feature r=kvark a=alteous

Addresses part of #156 .
bors bot added a commit that referenced this issue Jan 6, 2018
160: Add gltf-loader feature r=vitvakatu a=alteous

Addresses part of #156 .
@kvark
Copy link
Collaborator Author

kvark commented Feb 24, 2018

glTF is certainly not the reason of slow compile times on my machine. Non-gltf examples take 25 sec, while gltf-enabled ones take 30 sec. I'm going to rename this issue and leave #183 for follow-ups.

@kvark kvark closed this as completed Feb 24, 2018
@kvark kvark changed the title Optimize compile times Make glTF feature Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants