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

Added cylinder primitive #328

Merged
merged 6 commits into from Oct 10, 2020
Merged

Added cylinder primitive #328

merged 6 commits into from Oct 10, 2020

Conversation

tushar5526
Copy link
Contributor

@tushar5526 tushar5526 commented Oct 5, 2020

Closes #318
Referred to this source for generating vertices and triangles.
Formatted the primitive.py according to PEP 8.

@pep8speaks
Copy link

pep8speaks commented Oct 5, 2020

Hello @tushar5526! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 771:80: E501 line too long (87 > 79 characters)

Line 111:80: E501 line too long (82 > 79 characters)

Comment last updated at 2020-10-08 11:11:02 UTC

@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #328 into master will increase coverage by 0.10%.
The diff coverage is 95.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
+ Coverage   88.65%   88.75%   +0.10%     
==========================================
  Files          21       21              
  Lines        5075     5156      +81     
  Branches      656      672      +16     
==========================================
+ Hits         4499     4576      +77     
- Misses        406      408       +2     
- Partials      170      172       +2     
Impacted Files Coverage Δ
fury/primitive.py 91.54% <95.18%> (+2.15%) ⬆️

@skoudoro skoudoro added this to the v0.7.0 milestone Oct 5, 2020
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thank you @tushar5526 for this primitive!

we will look into it and give you more detailed comments. But for now, can you add some unit tests? You can look at https://github.com/fury-gl/fury/blob/master/fury/tests/test_primitive.py.

Thank you.

@tushar5526
Copy link
Contributor Author

Thank you for reviewing. Will add the tests soon :)

@tushar5526
Copy link
Contributor Author

tushar5526 commented Oct 6, 2020

@skoudoro, I am a little confused about how to implement the tests for this, I can assert the ndarray shape values, and do I have to check the e_mean, e_min, e_max by using hard-coded values?
PS: I have never written tests before so if there is anything I need to keep in mind, do tell me.
Thanks

@skoudoro
Copy link
Contributor

skoudoro commented Oct 7, 2020

Overall, it looks good. It will be good if you could change the default orientation of the cylinder (length along Y instead of Z).
See below:

Capture d’écran 2020-10-07 à 02 42 23

I created a PR in your fork to add some tests https://github.com/tushar5526/fury/pull/2. it is just missing a test when it is capped. I let you add this one last one.

fury/primitive.py Outdated Show resolved Hide resolved
fury/primitive.py Outdated Show resolved Hide resolved
fury/primitive.py Show resolved Hide resolved
tushar5526 and others added 3 commits October 7, 2020 12:29
Typo fixes.
Added check for sectors parameter.
Changed the base of cylinder from XY plane to XZ plane.
@tushar5526
Copy link
Contributor Author

Cylinder's base is on XZ plane now and the axes on Y.
Reproduced the mesh in unity for testing.
Screenshot from 2020-10-07 17-13-25

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

This PR looks good to me and is ready to go.

I will wait for any additional comments until tomorrow and then merge it.

@skoudoro skoudoro merged commit 7582333 into fury-gl:master Oct 10, 2020
@skoudoro
Copy link
Contributor

Thank you @tushar5526

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.

Cylinder primitive generation
3 participants