-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat: adapt ca85609 from MAO for the base engine #2612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies to multiple files (SettingsWindow, Strings, GameDatabase):
It would be preferable if on UI controls/preferences that this property is consistently after or before the original property/configuration name to keep parts of the same feature together.
Intersect.Client.Core/Core/Input.cs
Outdated
{ | ||
Globals.Database.WorldZoom = Graphics.MinimumWorldScale; | ||
if (wrap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine the 2 if statements
Intersect.Client.Core/Core/Input.cs
Outdated
Globals.Database.WorldZoom = Graphics.MinimumWorldScale; | ||
} | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set nextZoom
instead, and don't return
Intersect.Client.Core/Core/Input.cs
Outdated
if (wrap) | ||
{ | ||
Globals.Database.WorldZoom = Graphics.MaximumWorldScale; | ||
} | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as in HandleZoomIn
{ | ||
Core.Input.HandleZoomOut(false); | ||
} | ||
_lastZoomTime = DateTime.Now; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache DateTime.Now
in a local variable instead of calling it twice
|
||
{ | ||
|
||
if ((InputHandler.HoveredControl == null || !InputHandler.HoveredControl.IsVisibleInTree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this block scrolling on the canvas?
Shouldn't this or group include (InputHandler.HoveredControl is Canvas canvas && canvas == canvas.Canvas)
?
@@ -2089,6 +2089,9 @@ public partial struct Settings | |||
[JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | |||
public static LocalizedString EnableLighting = @"Enable Light Effects"; | |||
|
|||
[JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | |||
public static LocalizedString EnableMouseScrollZoom = @"Enable Mouse Scroll Zoom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be Enable scrolling to change World Scale in game
so that it's clear that it's the same feature
@@ -158,6 +161,7 @@ public virtual void SavePreferences() | |||
SavePreference(nameof(FullScreen), FullScreen); | |||
SavePreference(nameof(ShowFPSCounter), ShowFPSCounter); | |||
SavePreference(nameof(EnableLighting), EnableLighting); | |||
SavePreference(nameof(EnableMouseScrollZoom), EnableMouseScrollZoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be named EnableScrollingWorldZoom
to match the original feature's preference name
@@ -237,6 +237,9 @@ private void CheckMouseButton(Keys modifier, ButtonState bs, MouseButton mb) | |||
} | |||
} | |||
|
|||
private const double ZoomDelayInMilliseconds = 300; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to private static readonly TimeSpan ZoomDelay = TimeSpan.FromMilliseconds(300);
var deltaV = Math.Sign(mMouseVScroll - scrlVValue); | ||
|
||
var now = DateTime.Now; | ||
if ((now - _lastZoomTime).TotalMilliseconds >= ZoomDelayInMilliseconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the above change this becomes now - _lastZoomTime >= ZoomDelay
if (deltaV < 0) | ||
{ | ||
Core.Input.HandleZoomIn(false); | ||
} | ||
else | ||
{ | ||
Core.Input.HandleZoomOut(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these lines missing an indent or is this just the GitHub UI displaying it incorrectly?
This PR introduces mouse scroll functionality for zooming in and out of the game world.