Radius overlay#1946
Conversation
| constexpr unsigned HUNTER_SEARCH_RADIUS = 19; ///< Hunter searches for animals in a square of this half-side length | ||
| constexpr unsigned FARMER_RADIUS = 2; ///< Farmer plants and harvests crops within this radius | ||
| constexpr unsigned CHARBURNER_RADIUS = 3; ///< Charburner places and harvests charcoal piles within this radius | ||
| constexpr unsigned CATAPULT_RANGE = 12; ///< Catapult attack range |
There was a problem hiding this comment.
I see a if(distance < 14) check there. So why 12 here?
There was a problem hiding this comment.
Google said 12. :-( I started with my own numbers.
This is fixed.
There was a problem hiding this comment.
Also catapult was not available as constant, I extracted it to constant.
There was a problem hiding this comment.
Extracting constants is always good. You just have to make sure you actually use them after defining it.
@Spikeone I guess we should move to actually using radius for all jobs, not rectangles.
There was a problem hiding this comment.
Probably yes - can check in the original but pretty sure thats it's circular
There was a problem hiding this comment.
I see a
if(distance < 14)check there. So why 12 here?
Isn't it the same reason our building range is +1? So the target node is included or something like that?
There was a problem hiding this comment.
Which building range? if(distance < 14) means the building on the 13th node away can still be hit, so there are 12 nodes in-between. So maybe that is what is meant by google. And maybe the other part you are referring to has a corner case which required that. But would need to check
| // Draw at all 9 toroidal copies (canonical ± 1 map dimension). | ||
| // Using all copies guarantees the ring is continuous across the seam | ||
| // regardless of viewport position — the renderer clips off-screen pixels. | ||
| for(int dw : {-w, 0, w}) | ||
| { | ||
| for(int dh : {-h, 0, h}) | ||
| { | ||
| const MapPoint copyPt = MakeMapPoint(Position(basePt.x + dw, basePt.y + dh), mapSize); | ||
| const auto alt = world.GetNode(copyPt).altitude; | ||
| const DrawPoint scr = GetNodePos(copyPt) - DrawPoint(0, HEIGHT_FACTOR * alt) - offset; | ||
| Window::DrawRectangle(Rect(scr - DrawPoint(2, 2), Extent(5, 5)), BORDER_COLOR); | ||
| } | ||
| } |
There was a problem hiding this comment.
Noone commented on this yet.
The drawn radius was being clipped where map repeats and this is a workaround. Did not find any cleaner way advice appreciated.
There was a problem hiding this comment.
That is indeed hard. You can use the GetNodePos overload taking the height as the base point. Then use const Position mapDrawSize = world->GetSize() * Position(TR_W, TR_H); as the offsets
I don't think it gets better than that: You have to draw possibly all copies when the zoom factor is large which it seems is also not yet taken into account. Possibly even that is not enough.
Maybe rather use a (flat_)set to store the radius points and move drawing to DrawGUI where you can check with contains whether to draw the mark
| @@ -132,16 +132,14 @@ void nofHunter::HandleDerivedEvent(unsigned /*id*/) | |||
| void nofHunter::TryStartHunting() | |||
| { | |||
| // Find animals in a square around building (actually should be circle, but animals are moving anyway) | |||
There was a problem hiding this comment.
Uhm hunters look for animals in a SQUARE?
Should fix to be circle? Kinda out of scope but can include or do separate PR. Cause right now the range outline doesn't match what game uses.
There was a problem hiding this comment.
Created PR #1948 to make Hunters use circle instead of square.
|
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. |
4 participants
Add tooltip and overlay for building ranges