Fix ImageContainer2D for Fury v2: wrap image in UI and handle RGB images#1138
Fix ImageContainer2D for Fury v2: wrap image in UI and handle RGB images#1138VanshAgarwal24036 wants to merge 4 commits intofury-gl:v2from
Conversation
VanshAgarwal24036
force-pushed
the
port-imagecontainer2d-v2
branch
from
31f24f1 to
f7d396d
Compare
|
Hi maintainers All tests and ruff checks pass locally. Thank you! |
|
@maharshi-gor @ganimtron-10 plz review |
|
Hi @VanshAgarwal24036 , thanks for this PR, but, we are currently develpoing the v2 branch not the master branch, and given there is no conflicts with master I don't think is is based on v2, please always start your branch based on v2 branch not master. |
|
Thankyou @m-agour I will review again and do changes accordingly to v2 branch |
VanshAgarwal24036
force-pushed
the
port-imagecontainer2d-v2
branch
from
f7d396d to
d95620c
Compare
|
@m-agour please check now |
maharshi-gor
left a comment
maharshi-gor
left a comment
There was a problem hiding this comment.
Thank you @VanshAgarwal24036 for the work.
The PR requires better doc-strings and few feature support.
ganimtron-10
left a comment
ganimtron-10
left a comment
There was a problem hiding this comment.
Thanks for the PR. I noticed you closed a previous PR for the same thing, just fyi you don't need to create a new PR for new changes. If you commit to the same branch, it will automatically reflect in the existing PR.
I haven't done a deep dive yet, but here are my initial thoughts:
- Please include a demo using ImageContainer2D to make it easier to test the functionality.
- Don't create a separate file for tests, they should follow the existing project structure.
- Try to add more tests to validate all aspects of the UI, not just the happy path.
Please update the PR accordingly. Thanks!
|
Thanks for the helpful feedback! I’ve updated the PR according to the suggestions:
Also, since this is my first contribution to FURY, I initially got confused when the PR showed a very large number of commits. After looking into it, I realized that I had mistakenly based my branch on the wrong branch instead of Please let me know if any further improvements are needed. Thanks again for the review! |
|
@ganimtron-10 @maharshi-gor please tell me how to start workflow waiting |
VanshAgarwal24036
force-pushed
the
port-imagecontainer2d-v2
branch
from
b69a9af to
8691dc2
Compare
|
@maharshi-gor As my PR is failing with 20 checks so on your suggestion to @ganimtron-10 on his PR to rebase the branch and the checks pass as same problem in my PR also so now i have rebased so Please review it. |
|
Hi @maharshi-gor and @ganimtron-10 I have rebased the branch on the latest Could you please take another look when you get time? |
ganimtron-10
left a comment
ganimtron-10
left a comment
There was a problem hiding this comment.
Hi @VanshAgarwal24036 ,
Thanks for the PR!
PTAL at the below comments.
fury/ui/tests/test_containers.py
Outdated
| def test_image_container_rgb(): | ||
| """Test that RGB images are converted to grayscale.""" | ||
| img = np.zeros((10, 10, 3), dtype=np.uint8) | ||
| container = ui.ImageContainer2D(img) | ||
|
|
||
| assert container is not None |
There was a problem hiding this comment.
where are we testing rgb converted to grayscale?
…nstraints, update tests and example
|
Updated the implementation to align with the current backend constraints. |
4 participants
This PR fixes and ports
ImageContainer2Dto work properly with the current Fury v2 architecture.Fixes #1108
This PR:
_ImageUIclass that properly subclassesUI._update_actors_positionmethod.ImageContainer2D.