Repository navigation
Radius overlay - #1946
Radius overlay#1946morganchristiansson wants to merge 23 commits into
Conversation
|
Given #129 was a request for an addon, can you add one and guard the code changing the tooltip and setting the draw-circle by the addon setting? We usually keep the default behavior to be the same as in S2 BTW: Unless you just applied a suggested code snippet directly please don't resolve conversations as then we miss your reply |
Yes, I think so |
|
I think it should be player toggleable during game, and per player in multiplayer. But most ppl said it should be addon so maybe best to proceed with that. But should tooltip also be guarded by addon?
Ok I will leave resolving to original commenter from now on. I just did it to track what I resolved. |
|
nice idea, but please make it an optional addon. also, the red isn't very much in the style of S2, |
it is now an addon
not sure what graphics to use, please provide/suggest. |
|
Ok all issues solved. And made the addon enabled by default - mainly so I could use my existing save for testing. |
|
Found an edge case: it hovers game world buildings thru windows and will draw outline |
|
I have played many games with this enabled and it works perfectly. Hard to play without it. Only remaining issue is to use custom graphics instead of red rectangles. |
|
|
||
| // Auto-detect radius for the building under the cursor. | ||
| // Do not trigger hover when the mouse is over an ingame window. | ||
| if(!radiusPreview_ && GetWorld().GetGGS().isEnabled(AddonId::BUILDING_RADIUS) |
There was a problem hiding this comment.
Shouldn't that be rather on the outside, e.g. in dskGameInterface where it could be more efficiently in a MouseMove handler. This way it is opt-in and e.g. doesn't enable for observation windows (suspicion only, haven't checked)
And whereever this is done I'd say it makes sense to cache GetWorld().GetGGS().isEnabled(AddonId::BUILDING_RADIUS) (effectively const) instead of getting this on every draw call.
|
Addressed all feedback. |
|
|
||
| void GameWorldView::UpdateRadiusPreviewForMousePos(const Position& mousePos) | ||
| { | ||
| if(!isBuildingRadiusEnabled_ || WINDOWMANAGER.FindWindowAtPos(mousePos)) |
There was a problem hiding this comment.
Hm, this looks like this check should rather be handled by the caller: You update by selPt not mousePos and that is only set/updated in the Draw call so likely to be stale by 1 frame anyway.
Passing a value into this class just to check it using a different layer (world vs windows) feels wrong
Maybe we can turn it around: Add a method that gets the building type at the selection which we can reuse in dskGameInterface::ContextClick too, see existing code.
Do we really want to handle non-owned buildings? Feels like a cheat... If we do the code will be different so then I'd say a method getSelectedBuildingType here and handling the mouse pos/window-manager and buildings-enabled in the caller is cleaner.
60b85d4 to
76ce3f7
Compare
779c717 to
1469a6d
Compare
|
I resolved the conflicts & comments and tested it a bit. I haven't found a way to avoid this as the MouseMove on the desktop doesn't trigger when a modal window is open so the mouse moves on the map but doesn't show any radii. I'd say this glitch is fine. |
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
It uses `world` which only exists during a game. Make that explicit by taking the settings as a parameter
It is not constant as it may depend on game settings.
Calculating the points in a large radius in every draw call is costly. Do the constant part only once when the radius gets enabled.
When the desktop does not has the focus it won't update the radius preview. It then shows the last highlighted buildings radius when the selected point is close enough. Move the selected-building check back to GameWorldView and use the `showMouse` parameter as the sign whether to handle the mouse.
1469a6d to
2c4b1c1
Compare
4 participants
Add tooltip and overlay for building ranges