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 actor for cylinder #959

Merged
merged 7 commits into from
Jan 24, 2025
Merged

added actor for cylinder #959

merged 7 commits into from
Jan 24, 2025

Conversation

ManishReddyR
Copy link

Tried actor for cylinder

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.

Hi @ManishReddyR,

Thank you for this PR. Overall, it looks good.

@skoudoro
Copy link
Contributor

Also, it would be great to have a screenshot (with and without orientation and color)

@ManishReddyR
Copy link
Author

ManishReddyR commented Jan 21, 2025

Here are few of my screenshots

cyl_wc wor cyl_woc wr

@m-agour
Copy link
Contributor

m-agour commented Jan 22, 2025

Hi @ManishReddyR, PR looks good!
Please address the code format error here: https://github.com/fury-gl/fury/actions/runs/12895414278/job/35961872083?pr=959

@skoudoro
Copy link
Contributor

Also, comment the snapshot part for now until we find a fix for this function

ManishReddyR and others added 2 commits January 23, 2025 15:35
@ManishReddyR ManishReddyR requested a review from skoudoro January 23, 2025 21:08
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.

Hi @ManishReddyR,

See my review below. Please, pay attention to details if you want your PR to be merged fast.

Thank you

fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Show resolved Hide resolved
@ManishReddyR
Copy link
Author

Hi @skoudoro
Thanks for review, I also added the square actor to this pull request itself, is it better to make one more or let it be as it is.

Thank you

@skoudoro
Copy link
Contributor

in this case, it is ok.

But usually, better to split in multiple PR

@ManishReddyR
Copy link
Author

ManishReddyR commented Jan 24, 2025

Hello @skoudoro
I think, I have rectified all of the issues. Can you let me know anything more needed to be fixed.

Thank you

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.

Thanks for the update, see below

fury/actor.py Show resolved Hide resolved
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.

Thanks! all good, merging !

@skoudoro skoudoro merged commit c3d3c23 into fury-gl:v2 Jan 24, 2025
22 checks passed
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.

3 participants