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.

Sign up for GitHub

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

Jump to bottom

drag indicator node not showing #189

Closed
samreid opened this issue May 8, 2023 · 7 comments
Closed

drag indicator node not showing #189

samreid opened this issue May 8, 2023 · 7 comments
Assignees
@samreid
Labels
status:ready-for-review

Comments

@samreid
Copy link
Member

samreid commented May 8, 2023

While working on #172 and related to #129, the drag indicator node is not showing even after kicking all soccer balls.
@samreid samreid self-assigned this May 8, 2023
@samreid samreid mentioned this issue May 8, 2023
Closed
samreid added a commit that referenced this issue May 9, 2023
@samreid
Fix logic for showing the drag indicator node, see #189
75da2ce
@samreid
Copy link
Member Author

samreid commented May 9, 2023

Fixed, closing.

@samreid samreid closed this as completed May 9, 2023
@samreid samreid reopened this May 9, 2023
@samreid
Copy link
Member Author

samreid commented May 9, 2023

On second thought, maybe this should be code reviewed. @matthew-blackman or @marlitas can you please review?

@samreid samreid assigned marlitas and matthew-blackman and unassigned samreid May 9, 2023
samreid added a commit that referenced this issue May 9, 2023
@samreid
Update docs, see #189
dd415ab
samreid added a commit that referenced this issue May 10, 2023
@samreid
Add TODOs, see #189
4771623
@samreid samreid self-assigned this May 10, 2023
samreid added a commit that referenced this issue May 10, 2023
@samreid
Revert "Add TODOs, see #189"
75ec0ea 
This reverts commit 4771623.
samreid added a commit that referenced this issue May 10, 2023
@samreid
Add TODOs, see #189
15f8763
@samreid samreid removed their assignment May 10, 2023
@samreid
Copy link
Member Author

samreid commented May 10, 2023

@marlitas said she is working on this, self-unassigning.

marlitas added a commit that referenced this issue May 11, 2023
@marlitas
Move dragIndicator logic to model, see: #189
5370b5d 
# Conflicts:
#	js/common/view/CAVScreenView.ts
#	js/common/view/SceneView.ts
@marlitas
Copy link
Contributor

I moved it over, and I think it's better than being in the view, but I'm not sure I'm convinced by my implementation... I started with using DerivedProperties for both the dragIndicatorVisibleProperty and the dragIndicatorValueProperty, but that felt like it was getting expensive when the DerivedProperty had to listen to every soccer ball's valueProperty... I'm not sure this is any better though.

As I was testing this I was noticing that reset was taking longer than expected. Not sure if this is related to my changes or something else, but either way I'm worried about performance.

Would like to review with @samreid or @matthew-blackman.

@marlitas marlitas assigned samreid and unassigned marlitas May 11, 2023
@samreid
Copy link
Member Author

samreid commented May 11, 2023

I skimmed through the commit and feel it is the right direction. Reset already took a long time before these changes, and will probably need optimization. This issue seems a good one to review synchronously. I'll mark as help-wanted, and self-unassign for now.

@samreid samreid removed their assignment May 11, 2023
@samreid samreid added the dev:help-wanted Extra attention is needed label May 11, 2023
marlitas added a commit that referenced this issue May 11, 2023
@marlitas
update visibleProperty naming conventions, see: #189
311dd41
samreid added a commit that referenced this issue May 12, 2023
@samreid
Show the drag indicator above the stack if there is only one stack, see
59342b3 
#189
samreid added a commit that referenced this issue May 12, 2023
@samreid
Update soccer ball radii and stacking overlap, see #189
be8a990
samreid added a commit that referenced this issue May 12, 2023
@samreid
Simplify update drag indicator, see #189
16df3cc
@samreid samreid mentioned this issue May 12, 2023
Closed
@marlitas marlitas assigned samreid and unassigned matthew-blackman May 16, 2023
@marlitas marlitas removed the dev:help-wanted Extra attention is needed label May 16, 2023
@marlitas
Copy link
Contributor

@samreid would like to review this asynchronously.

@samreid
Copy link
Member Author

samreid commented May 16, 2023

Code review and sim behavior seem good, nice work! Closing.

@samreid samreid closed this as completed May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@samreid samreid

Labels
status:ready-for-review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@samreid @marlitas @matthew-blackman