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.

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

Adds scaling option for calibrators #783

Open
wants to merge 4 commits into
base: noetic
Choose a base branch
from

Conversation

onurtore
Copy link

New personal computers can handle the input without the need for downscaling, this commit makes it parametric.

delivers-26 added 2 commits November 18, 2022 20:16
Signed-off-by: delivers-26 <[email protected]>
@onurtore onurtore changed the title Adds scaling option Adds scaling option for mono calibrator Nov 18, 2022
@onurtore onurtore changed the title Adds scaling option for mono calibrator Adds scaling option for calibrators Nov 18, 2022
@onurtore
Copy link
Author

onurtore commented Dec 4, 2022

@JWhitleyWork,
Friendly ping.

@JWhitleyWork
Copy link
Collaborator

@onurtore Thanks for the PR. However, this pretty significantly changes the behavior of the node. I'm OK with the idea that you're presenting but not with the change in the default behavior. Instead, can you check for the existence of the parameter and, if it isn't defined, use the current behavior?

Signed-off-by: delivers-26 <[email protected]>
@onurtore
Copy link
Author

Hi @JWhitleyWork ,
You are right, I shouldnt have changed the default behaviour, many people using this node.

While I fixed that with my latest commit, I belive the current version of the code is a little bit problematic, see the scale parameter is the scale of the input image relative to 640x480, however this behaviour does not look like intuitive, I believe when someone use such parameter it would expect making scale >1.0 scales up the input image, however the current implementation decreases it, so maybe I should change how the scale param calculated. Anyway, I belive this version is still acceptable if its okay, I can create a seperate PR for what I mentioned

@onurtore
Copy link
Author

@JWhitleyWork,
Friendly ping.

1 similar comment
@onurtore
Copy link
Author

@JWhitleyWork,
Friendly ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants