-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
bevy_render: add torus and capsule shape #1223
Conversation
543430e
to
d6ccbd7
Compare
impl From<Icosphere> for Mesh { | ||
fn from(sphere: Icosphere) -> Self { | ||
if sphere.subdivisions >= 80 { | ||
// https://oeis.org/A005901 |
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.
It looks like oeis content is available under the Creative Commons Non-Commerical 3.0 license: https://creativecommons.org/licenses/by-nc/3.0/
This is unfortunately not compatible with the MIT license (or the general concept of selling games).
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.
the Icosphere
code is not modified in this commit, only extracted into an own file.
It looks like this commit introduces the comment.
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.
oof we should remove that then 😄
@OptimisticPeach any thoughts on this?
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.
Hmm, I think it'd be preferable to keep some kind of explanation there, however it's up to your discretion.
I'm not very keen on license rules, however might wikipedia be a better resource? https://en.wikipedia.org/wiki/Geodesic_polyhedron#Elements
I just read your second comment about a CC ShareAlike license. Let me look for a better way
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.
Okay, I drafted up a desmos graph to explain a method of calculating it:
https://www.desmos.com/calculator/8bbvlpkecj
However, it doesn't seem that desmos can be used for commercial applications or anything of the sort, hence, I made a gist with the same contents:
https://gist.github.com/OptimisticPeach/0851820de5694850d10f8b050beed125
I believe that linking to that, and additionally changing the code to reflect it, to the following:
let temp = sphere.subdivisions + 1;
let number_of_resulting_points = temp * temp * 10 + 2;
To better reflect these calculations.
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 changed the code and included @OptimisticPeach's explanation (in my opinion it is short enough to put it in the code). I think this PR should only contain MIT licensed code now.
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.
Sounds good to me. Thanks for resolving this so quickly!
|
||
impl From<Torus> for Mesh { | ||
fn from(torus: Torus) -> Self { | ||
// code adapted from http://wiki.unity3d.com/index.php/ProceduralPrimitives#C.23_-_Torus |
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.
Unity Wiki content is Creative Commons ShareAlike 3.0. This probably qualifies as a "remix", which means this code also must be released under that license.
We can probably deal with a ShareAlike license, but its suboptimal because any changes made to this code must also be ShareAlike, and we need to label this file as such. In general I'd prefer it if we used content that is compatible with MIT to keep the licensing situation clear.
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.
Right, I understand. I'll look for another implementation that's more permissively licensed. MIT and CC-0 should be okay, right?
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.
Yup CC-0 is fine. It doesn't require attribution, but if we find code, lets provide attribution anyway (if we can).
These changes are very welcome, but they currently come at the cost of license complexity (and/or incompatibility). I'm hoping we can find equivalent algorithms / number sequences under more permissive licenses. I'm guessing the folks at Unity Wiki didn't invent the Torus generation algorithm, so this might just be a matter of finding a more "primary" source and basing the code on that. |
I think the range loop is more understandable here.
878445b
to
b454bfc
Compare
Looks good to me. Thanks for putting up with my cautiousness. I just want to be as safe as possible when it comes to these things. |
* bevy_render: add torus shape * bevy_render: add capsule shape * bevy_render: reorganize shape module * bevy_render: add more docs
The file was getting quite large, so I also refactored it into multiple files.