-
Notifications
You must be signed in to change notification settings - Fork 236
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
Add feature to preview adjacent units when building #6283
base: develop
Are you sure you want to change the base?
Conversation
I'm open to suggestions on the name of the feature in the game options 😃 ! |
Example of dynamic scaling: unit-adjacency-preview-ratio.mp4 |
There could be a special case for non-template build mode, in which the build preview is located using mouse worldpos and mimicking however the engine interprets skirts and skirt offsets onto the world grid. |
You're right! I just checked again, I must have goofed up there. Would make for an interesting improvements in version 2, where we for example add an energy or mass icon if it impacts production or consumption.
I don't think the complexity of it is worth it. The feature is quite simple right now. That keeps it maintainable. |
@lL1l1 I've prepared the functions to allow them to be expanded in the (near) future. Could you give the pull request a review as it is right now? Then we can ship that, because I think it already adds sufficient value the way it is now. |
{ text = "<LOC _Off>", key = 0 }, | ||
{ text = "<LOC _On>", key = 1 }, |
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.
What's the standard for these option keys? We have numbers, strings, and booleans being used in options.lua
for simple on/off options.
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 don't know - I copied another on/off field and adjusted the text. I'm fine with any suggestion
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 true | false
makes sense for on/off options, and then false | string
for off/multichoice options. It avoids having to compare the key against 0 to see if the option is enabled, and when you have multiple choices the string makes it clear in the code where a certain choice is implemented. Numbers should be for when you are going to use the number directly in some calculation, just like a slider option.
Also it's better to use skirt size, currently T1 PD and walls have the same icon size as a mex. |
Processed the skirt size feedback in 5bd048c |
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Behavior at this moment: adjacency-preview-03.mp4 |
I think for this feature to be useful it should only show adjacency when there is an actual bonus. And then we probably should show only a mass or energy symbol. The way it is now it literally shows up on every combination of buildings which makes the feature a bit annoying. I can already see that my air factory is next to the land factory. I don't need to see the icon of the building at that moment. They are a bit hard to process anyway, so at the moment I don't see a significant benefit here. I'd say we should only ship this feature once we have it reduced to buildings where you actually care for adjacency otherwise a lot of players will probably turn it off in the beginning and then not even experience the version 2. Unrelated to that, this should probably point to dev iteration 3 anyway, right? |
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 left some more comments in this review and added to the discussion in the previous review.
I agree with BlackYps that it should point Dev iteration 3. It's a good proof of concept but it needs more polishing to be beneficial but unobtrusive for players.
local unitPosition = unit:GetPosition() | ||
local unitScreenPosition = worldView:Project(unitPosition) |
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.
local unitPosition = unit:GetPosition() | |
local unitScreenPosition = worldView:Project(unitPosition) | |
local unitScreenPosition = worldView:GetScreenPos(unit) |
Also the annotation for CUIWorldView:GetScreenPos
says Unit
instead of UserUnit
which looks like a mistake.
end | ||
|
||
-- label needs an initial position | ||
self:UpdatePosition(worldView, unit) |
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.
UpdateReferences
is called every frame, so this initial position update needs to be moved somewhere else where it runs once like the if
statement above, __init
, or the part where the label gets created and cached.
|
||
-- update internal state that we need during `OnFrame` | ||
self.Unit = unit | ||
self.WorldView = worldView |
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.
This would be a great place to add multi-worldview support. Currently if you play split screen it only shows the label on the left. Seems like you can just do
local mousePos = GetMouseScreenPos()
for _, view in import("/lua/ui/game/worldview.lua").GetWorldViews() do
if view:HitTest(mousePos[1], mousePos[2]) then
self.WorldView = view
end
end
In general it would be nice to have a function that gets the current worldview the cursor is on top of, an observer that tells when the worldview the cursor is on changes, and possibly a way to easily render something in all worldviews.
Multiview support can be applied for the area reclaim dragger too.
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.
and possibly a way to easily render something in all worldviews.
A user only looks at one place at a time however. From a performance perspective the function OnFrame
triggers at least 60 times per second, but it can be much higher if people unlocked the FPS.
I can see the value in using the world view the mouse is over, but I'm personally not in favor of always rendering the data on all worldviews and/or make that more accessible to developers because of the impact it can have.
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.
The engine already renders everything on all worldviews, so I think that such a capability should be open for developers so that they can maintain consistency with how the game normally plays. Of course it can come with a hefty warning that it may cost performance and it is unnecessary for one-time tasks where the user is focusing on where the cursor is and not looking at different views simultaneously.
But back to the PR topic, we can just render only in the current view of the mouse, and it would be nice for whoever plays some multiscreen setup, say if they have their base on one view and their front on the other view.
self.Left:Set(unitScreenPosition[1] - self.Width() / 2) | ||
self.Top:Set(unitScreenPosition[2] - self.Height() / 2) |
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.
For worldview support you need to offset from the worldview position.
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 do not understand, can you elaborate?
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.
CUIWorldView:Project
returns the screen space in local coordinates for the worldview, not absolute screen coordinates. viewleft
is located at (0, 0) so that works, but viewright
is located at (ScreenWidth/2, 0) so your labels won't be in the right place. Something like this:
ReusedLayoutFor(self)
:AtLeftTopIn(worldView
, unitScreenPosition[1] - self.Width() / 2
, unitScreenPosition[2] - self.Height() / 2
)
:End()
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.
We should probably update the description 😃 , even though it is there I did not immediately make that conclusion haha.
Description of the proposed changes
It's always been difficult to properly understand what units are considered adjacent when you are building. With this new feature we try to alleviate this problem!
With thanks to
OnDetectAdjacencyBonus
in gamemain.lua we've always had the ability to detect adjacency while constructing. But up until now we did not quite do anything with that ability. Now, we render the unit icon on top of all units that are adjacent.Note that the UI is scaled to 150% in the following preview
unit-adjacency-preview.mp4
Testing done on the proposed changes
Spawn in structures and then create all sorts of build previews.
Additional context
The feature is enabled by default. It can be disabled in the game options at
Interface -> HUD -> Show adjacent units when building
.There is no reliable solution for more advanced checks and/or drawings. This includes, but is not limited to:
The reason for this is because we only know what unit we're adjacent to but we do not know what unit in the build preview is adjacent to that unit. When there's only one build target this is trivial, but when we drag build and/or use a template it because much, much less trivial to understand. Therefore I opted for the simple solution to just always show the icon.
Checklist