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

Remove derivative from rendy #210

Closed
wants to merge 11 commits into from
Closed

Remove derivative from rendy #210

wants to merge 11 commits into from

Conversation

Veykril
Copy link
Contributor

@Veykril Veykril commented Sep 22, 2019

Removes the derivative dependency from crates that are using it, as mentioned in #203, replacing it with manual implementations of the corresponding traits.

@codecov-io
Copy link

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   87.57%   87.57%           
=======================================
  Files           6        6           
  Lines         169      169           
=======================================
  Hits          148      148           
  Misses         21       21
Impacted Files Coverage Δ
util/src/types/vertex.rs 86.3% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7cb21...fdd63ff. Read the comment docs.

@Frizi
Copy link
Member

Frizi commented Sep 23, 2019

I like the idea, but before we commit to writing impls manually i'd like to see actual numbers. Can you measure the build times from before and after this change?

@Veykril
Copy link
Contributor Author

Veykril commented Sep 23, 2019

Sure, and as it turns out the times are pretty much the exact same as it seems, regarding just building the rendy crates without the dependencies I got 20.03s and on a second try 24.53s for 9e7cb21 and 24.29s for my branch, and with all dependencies its 1m 08s for both, though for my branch derivative is still being build as dependency due to winit using it. So it seems derivative itself doesnt do much.

@Frizi
Copy link
Member

Frizi commented Sep 23, 2019

In that case I would prefer to get rid of failure first. Derivative macros are really useful for maintenance reasons, so if there is no real difference in build times I'd like to keep them.

@Veykril
Copy link
Contributor Author

Veykril commented Sep 23, 2019

Ye figured, definitely the smarter choice to stick with derivative then for the time being. Will try to see what removing failure will do and if it improves the times or not, depending on that I'll do a PR or comment in issue #203 with my findings.

@Veykril Veykril closed this Sep 23, 2019
@Veykril Veykril mentioned this pull request Sep 23, 2019
@Frizi
Copy link
Member

Frizi commented Sep 23, 2019

Okay, thank you for doing that @Veykril :)

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

Successfully merging this pull request may close these issues.

3 participants