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

Fit resolutions to less than 2k x 2k #1065

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Conversation

iterion
Copy link
Contributor

@iterion iterion commented Nov 13, 2023

I put in limits on the API side to prevent huge resolutions. But, we still have a few cases where modeling app will request a bigger resolution, like full screen on my biggest monitor.

An unfortunate side-effect of this is that we'll also need to scale the resolution of all mouse events, but I'll do some testing to be sure.

Copy link

vercel bot commented Nov 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Nov 13, 2023 9:12pm

@mlfarrell
Copy link
Contributor

You may have to do more here than just limit the res. I suspect aspect ratios will come into play and also accounting for this weird additional scaling factor in the web -> engine 2D space conversions

@iterion
Copy link
Contributor Author

iterion commented Nov 13, 2023

Yeah, we definitely will, I tried it out and mouse events definitely get offset. I'll have to think more about how to refactor this first.

@Irev-Dev
Copy link
Collaborator

There's a chance that this will work without having to worry about mouse coords, assuming the correct stream width and height are fed into getNormalisedCoordinates (src/lib/utils.ts:80). If not that's probably the place to look to fix anyway.

@iterion
Copy link
Contributor Author

iterion commented Nov 13, 2023

Oh nice, thanks for the tip, I'll give it a shot.

@iterion
Copy link
Contributor Author

iterion commented Nov 13, 2023

Ok, that worked for scaling the input, now I just want to remove some of the duplication here. I'm wondering instead if we should do this scaling when we setStreamDimensions instead.

@Irev-Dev
Copy link
Collaborator

Ok, that worked for scaling the input, now I just want to remove some of the duplication here. I'm wondering instead if we should do this scaling when we setStreamDimensions instead

That sounds like a good bet to me.

Copy link
Collaborator

@Irev-Dev Irev-Dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very elegant change in the end 🏆

@iterion iterion merged commit 2519712 into main Nov 14, 2023
16 checks passed
@iterion iterion deleted the fit-resolution-below-2k-by-2k branch November 14, 2023 00:32
@pierremtb pierremtb mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants