-
Notifications
You must be signed in to change notification settings - Fork 96
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 Three Different Cases for Adding a New Instance #1859
Conversation
WalkthroughThe modifications enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VideoWidget
participant ContextMenu
participant Command
User ->> VideoWidget: Right-click for context menu
VideoWidget ->> ContextMenu: Display options
ContextMenu ->> User: Show "Default" option
User ->> ContextMenu: Select "Default"
ContextMenu ->> VideoWidget: Trigger newInstance(init_method="best", location=scene_pos)
VideoWidget ->> Command: create_new_instance(location=scene_pos)
Command ->> Command: Adjust node positions based on location
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1859 +/- ##
===========================================
+ Coverage 73.30% 74.34% +1.03%
===========================================
Files 134 135 +1
Lines 24087 24755 +668
===========================================
+ Hits 17658 18405 +747
+ Misses 6429 6350 -79 ☔ View full report in Codecov by Sentry. |
…ion of the cursor
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/gui/commands.py (4 hunks)
- sleap/gui/widgets/video.py (1 hunks)
Additional comments not posted (5)
sleap/gui/widgets/video.py (1)
370-373
: LGTM! Ensure the usage ofscene_pos
andnewInstance
method in the codebase.The code changes are approved.
However, verify the usage of
scene_pos
andnewInstance
method to ensure correctness and consistency.Verification successful
The usage of
scene_pos
andnewInstance
method is correct and consistent in the codebase.The code changes are verified and approved:
scene_pos
is properly used as a parameter to thenewInstance
method in multiple instances withinsleap/gui/widgets/video.py
.- The
newInstance
method insleap/gui/commands.py
correctly handles thelocation
parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `scene_pos` and `newInstance` method in the codebase. # Test: Search for the usage of `scene_pos` in the codebase. Expect: Proper handling of `scene_pos`. rg --type python -A 5 $'scene_pos' # Test: Search for the usage of `newInstance` method in the codebase. Expect: Proper handling of `newInstance` with `location` parameter. rg --type python -A 5 $'newInstance'Length of output: 131
Script:
#!/bin/bash # Description: Verify the usage of `scene_pos` and `newInstance` method in the codebase. # Test: Search for the usage of `scene_pos` in the codebase. Expect: Proper handling of `scene_pos`. rg -A 5 'scene_pos' # Test: Search for the usage of `newInstance` method in the codebase. Expect: Proper handling of `newInstance` with `location` parameter. rg -A 5 'newInstance'Length of output: 6976
sleap/gui/commands.py (4)
2916-2916
: Verify the addition of thelocation
parameter.The addition of the
location
parameter to the function signature appears correct. Ensure that all calls tocreate_new_instance
in the codebase are updated to handle this new parameter.
2988-2988
: Verify the handling of thelocation
parameter.The handling of the
location
parameter appears correct. The logic for setting node positions based on thelocation
offset is sound. Ensure thatlocation
is properly passed and utilized in all relevant scenarios.
3015-3015
: Verify the addition of thelocation
parameter.The addition of the
location
parameter to the function signature appears correct. Ensure that all calls toset_visible_nodes
in the codebase are updated to handle this new parameter.
3030-3038
: Verify the logic forlocation
based node positioning.The logic for adjusting node positions based on the
location
offset appears correct. Ensure that thelocation
parameter is properly passed and utilized in all relevant scenarios.
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.
Have not gotten a chance to test yet, but some comments. Most important is how the offset will be affected when scaling a few lines below. AND also passing in the init_method
to determine whether to offset.
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/commands.py
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 current implementation will add (a scaled) +10 offset even if we are in the third case
- Right click and click "Default" to create instance should make the instance based on the location of the click
or the unmentioned fourth case
- Neither cases 1-3
We want to add an unscaled +10 offset on in the first case:
- Ctrl + I should create instances slightly away from the copied instance
And also handle the fourth case better (i.e. 0 offset). See suggestions/comments
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/commands.py
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/commands.py
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/commands.py
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 pushed a few changes. Do you mind reviewing? The thing I am most uncertain about is setting the OOB nodes to not visible after shifting them in bounds.
reference_node = next( | ||
(node for node in copy_instance if not node.isnan()), None | ||
) |
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.
Finds first non nan reference node
if x_new_offset < 0: | ||
x_new = 0 | ||
visible = False | ||
elif x_new_offset > new_size_width: | ||
x_new = new_size_width | ||
visible = False | ||
else: | ||
x_new = x_new_offset | ||
if y_new_offset < 0: | ||
y_new = 0 | ||
visible = False | ||
elif y_new_offset > new_size_height: | ||
y_new = new_size_height | ||
visible = False | ||
else: | ||
y_new = y_new_offset |
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.
Moves out of bounds nodes to closest in bounds position. Also marks those nodes that were going to be out of bounds as not visible.
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/commands.py
Description
Types of changes
Does this address any currently open issues?
This PR is an expansion of PR #1816.
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
location
for node positions, enhancing control over instance initialization and visibility settings.location
.