-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add fill tool #4780
Add fill tool #4780
Conversation
- fix floodfill for all dimensions
frontend/javascripts/oxalis/controller/combinations/volumetracing_plane_controller.js
Outdated
Show resolved
Hide resolved
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.
Very cool stuff!
Unfortunately, the feature didn't really work for me during testing. Not sure whether I produced some edge case (let me know if I should show it to you in a call), but I also left a comment regarding infinite loops which might be relevant?
Also, see my other feedback for improving code readability :)
frontend/javascripts/oxalis/controller/combinations/volumetracing_plane_controller.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
If I don't trigger the already mentioned edge case (which I seemed to have done all the time when first checking this out), this feature is a lot of fun :) Especially in combination with volume-undo. Very well done!! 🥇 |
@philippotto I just fixed the bug and applied your feedback as good as I could. Could you please try out the newest version, once the CI has finished? |
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.
Awesome, awesome, awesome 👍 I left some other minor feedback, but afterwards, this should be ready to get shipped 🚢
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js
Outdated
Show resolved
Hide resolved
I think I just applied all of your feedback and tested the basic use cases. Everything should work. @philippotto Could you please recheck this PR (after the CI is done) 🕺 |
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.
Top notch :)
This PR adds a flood fill tool to the brush.
TODOS:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated (unreleased) migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment