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

Static cropping #1610

Open
wants to merge 60 commits into
base: main
Choose a base branch
from
Open

Static cropping #1610

wants to merge 60 commits into from

Conversation

GGreenix
Copy link

fixed the offset problem with the tags

@GGreenix GGreenix requested a review from a team as a code owner November 29, 2024 21:57
@mcm001
Copy link
Contributor

mcm001 commented Nov 29, 2024 •
edited
Loading

What's the difference between this and 1602?

@GGreenix
Copy link
Author

Idk why but the other branch broke for me for some reason, this one has all of the necessary functionalities, After this will go mainstream I will research about dynamic cropping

@mcm001
Copy link
Contributor

mcm001 commented Dec 6, 2024

Were comments from #1602 addressed?

@GGreenix
Copy link
Author

GGreenix commented Dec 6, 2024

Were comments from #1602 addressed?

You can close this one 1610 is more relevant

@mcm001
Copy link
Contributor

mcm001 commented Dec 6, 2024

You can close this one 1610 is more relevant

Sure. did you address our review comments from 1602?

@GGreenix
Copy link
Author

GGreenix commented Dec 8, 2024

You can close this one 1610 is more relevant

Sure. did you address our review comments from 1602?

yes I just ditched this part with the dynamic cropping mechanism and will put more effort to it after we make the static cropping work, I will start from zero without breaking the project

@mcm001
Copy link
Contributor

mcm001 commented Dec 9, 2024

Before I review the changes, did you address the other comments as well?

@crschardt
Copy link
Contributor

crschardt commented Dec 9, 2024 •
edited
Loading

The index.html file is in the repo, but should not be tracked or committed. I'm sorry for the messy commits, but I forgot how to remove it once it was already in a PR and it took a couple tries for me to get it right.

@amquake amquake mentioned this pull request Dec 10, 2024
Closed
@GGreenix
uncropping pipe and more
3b2fa8d 
added an UncropApriltagsPipe which creates new AprilTagDetection with the new offsets.

Added width and height of settings to cropping pipe instead of max integer
@GGreenix
forgot to delete old offset method
d33d67f
@crschardt
Copy link
Contributor

When a pipeline is cropped, switching to driver mode and then back causes the crop window to shrink to minimum settings (0,0,16,16). The crop pipe needs to remember its settings when toggling driver mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crschardt crschardt crschardt requested changes

@amquake amquake amquake left review comments

@Gold856 Gold856 Gold856 requested changes

@spacey-sooty spacey-sooty spacey-sooty requested changes

@mcm001 mcm001 mcm001 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@GGreenix @mcm001 @crschardt @amquake @spacey-sooty @Gold856 @Alextopher