Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add option to adjust viewport zoom rate in the preferences dialog #2420

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Sidharth-Singh10
Copy link
Contributor

Closes #2411

let log_val = rate.ln();

// Map to 1-100 range
let normalized = (log_val - log_min) / (log_max - log_min);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Keavon when you have a moment , could be please take a look if the formula of logarithmic scaling correctly implemented for our use case.

I took reference from here.

This PR sets the minimal possible zoom rate to 0.0001 and max possible to 0.05
the default value of 0.005 will correspond to 63 on the frontend

@Sidharth-Singh10 Sidharth-Singh10 marked this pull request as ready for review March 14, 2025 15:46
Copy link

📦 Build Complete for 64c30d2
https://e1bdf74f.graphite.pages.dev

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

This appears to work nicely; thanks for working on this.

@Keavon
Copy link
Member

Keavon commented Mar 14, 2025

My concern is that the 63 default feels arbitrary. I would prefer a default of 50 and a draggable number input slider between 1 and 100. To be clear though, I'm not talking about changing the rate already present in master. 50 should map to what is currently 63. or to borrow from your comment above:

This PR sets the minimal possible zoom rate to 0.0001 and max possible to 0.05 the default value of 0.005 will correspond to 63 on the frontend

This means whatever is 50, not 63, should become 0.005 and then it should be able to grow by a reasonable amount to reach 100 and shrink by a reasonable amount to reach 1.

@Sidharth-Singh10
Copy link
Contributor Author

Fixed it!

@@ -307,15 +307,15 @@ pub fn map_linear_range(value: f64, from_min: f64, from_max: f64, to_min: f64, t

// Map the actual zoom rate value to display value (1-100)
fn map_zoom_rate_to_display(rate: f64) -> f64 {
map_log_range(rate, 0.0001, 0.05, 1.0, 100.0).round()
map_log_range(rate, 0.000273, 0.0972, 1.0, 100.0).round()
Copy link
Member

@Keavon Keavon Mar 14, 2025

Choose a reason for hiding this comment

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

Can you write out the equations which produce these numbers, instead of just writing their magic number results? Also stylistically, please end all your whole-number floats with . instead of .0 (for 1.0 -> 1. and 100.0 -> 100.).

Copy link
Contributor Author

@Sidharth-Singh10 Sidharth-Singh10 Mar 15, 2025

Choose a reason for hiding this comment

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

using the equation r = exp(ln(min_rate) + (d-1)/(100-1) * (ln(max_rate) - ln(min_rate)))
where r (0.005) is the zoom_wheel_rate and and d is the display value (50),
this creates one equation with two unknowns, and I have only one equation, we would need another pair of r and d.
hence why i used the magic numbers there. Initially I just went with the feel for it, but as you mentioned to map 0.005 to 50, so I used a LLM to produce the magic numbers

Copy link
Member

@Keavon Keavon Mar 15, 2025

Choose a reason for hiding this comment

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

If it's a nontrivial problem to work out a solution for, I can guarantee that the numerical solution an LLM produced will be wrong. I'm also a bit confused, you're saying that it's one equation with two unknowns? Then how are you expecting to get any kind of solution (other than a hallucinated one) in that case? Surely there should be a solution to whatever equation needs to describe this situation, so could you try working that out (maybe with the help of Wolfram|Alpha and/or Desmos)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the values generated by the LLM are fairly accurate. The display value of 50 consistently maps to an actual rate of 0.005 which suggests a reliable pattern. The approach involved iterating over pairs of max_rate and min_rate,narrowing down the lower bound to values between 0.0001 and 0.001 and the upper bound to between 0.05 and 0.2. I think this method appears to work correctly. That said I agree that the numbers are arbitrary, will try to find possible equation
Screenshot_20250315_153355

Copy link
Member

Choose a reason for hiding this comment

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

Conceptually, you have your logarithmic scaling and we have our desired middlepoint where 50 (an arbitrary scale) maps via your log equation to 0.005. From that point, we can go left by 49 units to reach 1 and right by 50 units to reach 100. That means, via the log scaling, we travel towards slowness and fastness until 1 maps to something slow and 100 maps to something fast. The rate of change is all that we need to decide upon such that the 1 and 100 both happen to be approximately the minimum and maximum speed anyone would ever want to use. So whatever your equation is shouldn't really matter, just the log scaling, the initial value where 50 becomes 0.005, and a constant we decide for how far the 49 and 50 should really go into the realm of slowness and fastness. The values you write in code should just be 1, 50, 100, 0.005, and a chosen rate of change. If you pick a sensible rate of change, I can help comment on whether it should be greater or less.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it!
I pushed changes, now we just have a single VIEWPORT_ZOOM_WHEEL_RATE_CHANGE to tweak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the current value of VIEWPORT_ZOOM_WHEEL_RATE_CHANGE ( 3. ) the max rate is 0.1 and min rate is 0.0002

@Keavon
Copy link
Member

Keavon commented Mar 15, 2025

!build

Copy link

📦 Build Complete for 9d41a45
https://2260030a.graphite.pages.dev

@Sidharth-Singh10 Sidharth-Singh10 requested a review from Keavon March 19, 2025 08:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preference to adjust zoom rate
3 participants