Feature: register new lua function DrawBuildSquare to show build squares on custom building commands

[aurelienlt]

Registers Spring.AddBuildSquare(unitDefId, x, y, z, facing [, opts]) and Spring.RemoveBuildSquare(unitDefId, x, y, z, facing) which display the usual grid.

Options are

• unbuildable=false: show the building as unbuildable (orange) instead of buildable (green) even if there is no detected conflict.
• defaultColor={0,0,0,0}: replace all the hardcoded color with this given color (then it will be overriden by next colors)
• buildableColor={0,0,0,0}: change the color for buildable (green)
• unbuildableColor={0,0,0,0}: change the color for unbuildable (orange)
• featureColor={0,0,0,0}: change the color for feature (orange)
• illegalColor={0,0,0,0}: change the color for illegal (red)

Mostly forking CUnitDrawerLegacy::ShowUnitBuildSquare.

My first MR on that project, so please lemme know best practices.

[sprunk]

I think drawing should not be doing any validity calculations (add another function that checks overlap, buildability or whatnot) and should draw a single blueprint.

[aurelienlt]

This was a copy of the behvior of CUnitDrawerLegacy::ShowUnitBuildSquare.

So you think that we should have a function returning the state of every square associated with a building position, and another function that displays each of this square ?

How would this change help developping scripts that use the building square feature ?

[sprunk]

This was a copy of the behvior of CUnitDrawerLegacy::ShowUnitBuildSquare.

Sure, but Lua interface design shouldn't depend on that.

So you think that we should have a function returning the state of every square associated with a building position, and another function that displays each of this square ?

Not necessarily. I just think each blueprint circled purple here should be drawn via 1 lua call that accepts unitDefID, XZ position, and facing. Engine would take care or drawing the ghost and the individual yardmap squares green/yellow/red as appropriate.

Functions to test buildability exist already (Spring.TestBuildOrder, overlap is just rectangle math).

How would this change help developping scripts that use the building square feature ?

By having a clean API. Drawing a blueprint of unit A at position B should generally involve passing A and B to a function, nothing more, regardless of engine internals. There is no reason for it to accept unitIDs or to accept the rest of the input specifically in the command format (which comes with extras like command options). There may be a reason to have a version that accepts an array instead of a single one if it turns out to be a perf problem but I am unconvinced.

[aurelienlt]

OK that's quite clear now. I'll work on improving that.

[aurelienlt]

@sprunk OK I've rewritten the MR to feature a more simple and direct function. Also added some options to allow to edit the colors, I'm thinking about templates which have a specific color for buildings not buildable by the current builder. I explain that in the description. Lemme know if it's overkill.

[sprunk]

1. It should accept unitDefID, x, z, facing and not buildCommand. The interface is more generic than just build commands (e.g. you can draw some sort of "reinforcements will spawn here, clear the way" markers).

2. Native GUI colors are generally defined via "cmdColors", add color customisation there.

3. bool unbuildable looks good.

[sprunk]

@GoogleFrog you originally came up with the suggestion to do it like this so maybe you have some thoughts on API design as well.

[aurelienlt]

I used commands because it avoided declaring the type buildInfo in lua (command is bascally {-unitDefID, {x, y, x, facing}, opts}). But I guess simplifying the type could have some usage.

I also thought about the need of displaying arbitrary squares, but there a caveat to that: the buildability and the height of a square depends on the type of building (some are on or under water, walls can be built on any slope ...). So it would be a different interface anyways that needs to define its own height for each square.

I didn't know about cmdColors, better place to define those hardcoded colors. Not sure if we can use it for opts' colors, because the key for custom colors seems to be a commandId. So that would require to register a comand ids CMD_CANNOT_BUID_HER_FOR_DIFFERENT_REASON, CMD_BUILD_HERE_IS_GOOD_IDEA ot CMD_PLEASE_CLEAR_THIS_AREA just to use some specific colors. I think directly passing colors is more simple to use.

I see some LUA calls like SetLosViewColors or SetAtmosphere to configure colors, so another options could be to add a call to configure default square colors and add arbitrary color labels for specific square displayed.

[aurelienlt]

I like the idea to draw arbitrary zones to show that a team member plans to use it, or that a zone should be defended. But visually I think it would make more sense to draw it as a polygon with player color borders and lighter color fill. some extension of the map draw, but as a filled shape instead of a line.

[sprunk]

I used commands because it avoided declaring the type buildInfo in lua

Well unitDefID, x, z, facing is just four numbers so there aren't any issues with types either.

Not sure if we can use it for opts' colors, because the key for custom colors seems to be a commandId. So that would require to register a comand ids CMD_CANNOT_BUID_HER_FOR_DIFFERENT_REASON, CMD_BUILD_HERE_IS_GOOD_IDEA ot CMD_PLEASE_CLEAR_THIS_AREA just to use some specific colors.

You can just add an arbitrary color entry there. Like here it's using some "restart" color directly which isn't associated with any command.

spring/rts/Game/SelectedUnitsHandler.cpp

Line 913 in 24efb38

cmdColors.restart,

I see some LUA calls like SetLosViewColors or SetAtmosphere to configure colors, so another options could be to add a call to configure default square colors and add arbitrary color labels for specific square displayed.

There is a Lua call to reload cmdcolors. It has a pretty bad interface that I'm unhappy about but that is outside the scope of this PR.

the buildability and the height of a square depends on the type of building (some are on or under water, walls can be built on any slope ...). So it would be a different interface anyways that needs to define its own height for each square.

I'm not asking for completely arbitrary squares, just the ability to draw the existing grid for specific buildings. You'd always have to pass a specific unitDefID and then it would draw squares as appropriate for that type. Pass the ID of a floating/underwater building and it will draw squares on/under water accordingly. Pass a wall's ID and it will draw OK for any slope, etc.

[aurelienlt]

OK I've replaced all the hardcoded colors with colors from cmdColors and updated the API to take unitDefId, x, y, z, facing. About all those xxxColor options, we still keep them or I get rid of them ?

[aurelienlt]

Oh also I wanted to put those ParseBuildInfo functions in LuaUtils but it fails to compile. I assum because LuaUtils is imported somehwere that doesn't include the code for BuildInfo. I guess it can be fixed by a #if ... but I have no idea what to check for.

./spring/rts/Lua/LuaUtils.cpp:1820: error: undefined reference to 'BuildInfo::BuildInfo(int, float3 const&, int)'
./spring/rts/Lua/LuaUtils.cpp:1863: error: undefined reference to 'BuildInfo::BuildInfo(int, float3 const&, int)'
/usr/include/c++/13/bits/stl_construct.h:97: error: undefined reference to 'BuildInfo::BuildInfo()'
/usr/include/c++/13/bits/stl_construct.h:97: error: undefined reference to 'BuildInfo::BuildInfo()'

[GoogleFrog]

@GoogleFrog you originally came up with the suggestion to do it like this so maybe you have some thoughts on API design as well.

The discussion looks good. I am not sure what Spring.DrawBuildSquareArray is for though.

[sprunk]

Could add a FIXME code comment that this is partially a copypaste of function X from file Y.cpp done due to compilation error Z

[aurelienlt]

I think you meant ParseBuildInfo, it has been commented

[sprunk]

IIRC there's some sort of facing parser in LuaUtils that makes facing args work both with numbers and things like "south", might want to optionally check it out

[aurelienlt]

No, all functions using facing (including SetBuildFacing) just parse an int.

[lhog]

Looks like DrawBuildSquare is supposed to be called every draw frame. Is it really the best way to to do it? AddLuaBuildSquare() does quite a bit of calculations, though put into the cache. Won't it be better to introduce some sort of state into that call, e.g.

1. AddDrawBuildSquare/DelDrawBuildSquare
2. encode how often you want to refresh if the state is current per DrawBuildSquare, the default will be at 1 synced frame.
3. SetLuaDrawBuildColors() to control the colorization
4. (Maybe) for folks like ZK it will be nice to also allow for supplying the desired abs or relative height values of the grid or height function similar to how Spring.SetHeightMapFunc() operates.

[aurelienlt]

This is actually the behavior of the original function ShowUnitBuildSquare: being called each rendering. Unless Update is called more often than Draw, I'm not fully clear about the enegine, but it feels like they are called as much, except when speeding a replay, in which case there is no square to show. Maybe there is a better moment to call DrawBuildSquare than in Update, it just seemed convenient since this is where building positions would be calculated by lua (this is the case for templates for instaces).

The squares are cached, so having Add/Del would complexify the behavior quite much for a limited gain. Especially since one player would only see HIS squares, only while holding the mouse button, and they would (almost) always be on screen.

About SetLuaDrawBuildColors, I'm not sure what would be the behavior. If the point is to customize some colors, I think it would make more sense to have SetCmdColors(colorsMap) -> originalColorsMap call that would let lua replace any color listed in CCommandColors::colorNames.

About custom height, I'm not sure what would be the use case. The current building command resolves height automatically.

[lhog]

ShowUnitBuildSquare is not a great example as it updates the state and draws at the same time. One evidence is that ugly cache vector that was introduced some time ago to make it work a bit faster. In fact the state should be updated only once per synced frame (perhaps even less frequently) or whenever the mouse moves.

The squares are cached, so having Add/Del would complexify the behavior quite much for a limited gain. Especially since one player would only see HIS squares, only while holding the mouse button, and they would (almost) always be on screen.

It will introduce some flexibility as in time we can figure out how to update the state smarter than just brute-forcing TestUnitBuildSquare() on every opportunity. For now at least users will be able to control how frequently the state should get brute-forced, unlike how it's done currently in the PR, where it's fixed to be CACHE_VALIDITY_PERIOD

Disregard my SetLuaDrawBuildColors comment, I misread how it's implemented in your PR.

About custom height, I'm not sure what would be the use case. The current building command resolves height automatically.

ZK has a concept of terrain terraforming, someone might want to draw the grid at the future height of X or even with some custom height shape.

[sprunk]

(Maybe) for folks like ZK it will be nice to also allow for supplying the desired abs or relative height values of the grid or height function similar to how Spring.SetHeightMapFunc() operates.

The current interface in the PR is already good because it lets you specify position as XYZ. So it would be a matter of the renderer not ignoring the Y. Perhaps if that happens then Y could become optional so that specifying nil makes it use the unsynced ground height.

[aurelienlt]

OK I understand the idea of Add/Del, it would permit for later optimisation. Looking at TestBuildSquare I'm not exactly sure what make it slow, seems like most steps are just about looking for values in 2D arrays. I suppose the best move would be to run this in a seperate thread, so it would draw stuffs once it has finished computed values, while displaying the old values when it takes longer than a frame.

For terraforming, unfortunately ShowUnitBuildSquare cannot really apply because I imagine that terraforming isn't associated to a unitDefId. It's more about adding the possibility to display arbitrary squares with arbitrary color.

About skipping the GetBuildHeight step, I can remove it from AddLuaBuildSquare since lua is already using GetBuildHeight, I'll have to check little more to know if I can remove it from TestUnitBuildSquare because it is called in quite many places.

[aurelienlt]

I've checked how this is implemented in VAO:

• all submiussions are store in an array
• when adding, it returns the added index
• when removing, it replace the element by the tail
• lua has to make a complicated dance of maintaining the right index to know how to remove

So do we want to mimic that implementation, or we should simplify using a map ? I feel like using a map is better because we can trust C++ to do a faster job at updating a map than lua is doing.

[aurelienlt]

OK next iteration. Now have AddBuildSquare and RemoveBuildSquare. Everything is stored in an unordered_map that also serves as cache (elements are kept few frames more). Calls to TestUnitBuildSquare are performed in the draw phase. That should make spamming AddBuildSquare much less dangerous because because it will only prduce map access until the draw phase. And obviously it will make it possible to optimize calls to only add / remove what is required.

[lhog]

I'm not exactly sure what make it slow, seems like most steps are just about looking for values in 2D arrays.

By itself is not that expensive (although it's much more than just a lookup in 2D array), but when N constructors queue M buildings it's NxM runs to perform and it becomes very expensive.

I suppose the best move would be to run this in a seperate thread, so it would draw stuffs once it has finished computed values, while displaying the old values when it takes longer than a frame.

This is in line with what I'm saying. Even if implemented naively, the only place something might change is at game frame time. The game runs 30 sim frames per seconds, while rendering frames normally reach 3 digit figures.

For terraforming, unfortunately ShowUnitBuildSquare cannot really apply because I imagine that terraforming isn't associated to a unitDefId. It's more about adding the possibility to display arbitrary squares with arbitrary color.

Terraforming defines the final state of heightmap, but it's ok if you skip this one.

I've checked how this is implemented in VAO

Unsure how VAO relates to the topic of discussion. But that actually led me to some interesting thought. The way we store the buildable/unbuildable/feature squares is very expensive and redundant. It's enough to store the build-ability data in a 2D value array, possibly a bitmap and use shader to colorize the output according to the chosen colors.
I may try to implement it today for ShowUnitBuildSquare().

[lhog]

OK next iteration. Now have AddBuildSquare and RemoveBuildSquare. Everything is stored in an unordered_map that also serves as cache (elements are kept few frames more). Calls to TestUnitBuildSquare are performed in the draw phase. That should make spamming AddBuildSquare much less dangerous because because it will only prduce map access until the draw phase. And obviously it will make it possible to optimize calls to only add / remove what is required.

Ok, I'll have a look today.

[aurelienlt]

The way we store the buildable/unbuildable/feature squares is very expensive and redundant. It's enough to store the build-ability data in a 2D value array, possibly a bitmap and use shader to colorize the output according to the chosen colors.
I may try to implement it today for ShowUnitBuildSquare().

Oh yeah I absolutely hate how the test function takes 3 vector pointers, instead of just returning a struct with a 2D array of states.

The game runs 30 sim frames per seconds, while rendering frames normally reach 3 digit figures.

I've been trying to understand how threading works, but I didn't get much clues through the code. I just noticed that despite having a very decent laptop, replay speed barely exceeds normal speed on big battles. I suppose this is the cause of unsynchronisations that players seem to complain about.

[aurelienlt]

Hi, anything else you want me to improve, or we're good to merge ?

[sprunk]

Documentation would be good:

• what happens if you call Add twice with the same args, or if the Y is different? How lenient is duplicate detection?
• do x/z need to be aligned to the grid?
• what is "cache validity"? Just the rate of buildability checks or will the square remove itself?
• does Remove require 100% identical args that were passed to Add to successfully remove? If not, how much leniency is there?

[sprunk]

Seems to be a table now.

[aurelienlt]

I've tried to fix those comments, I'm still not clear how they work though because they are quite different from LUA annotations.

[sprunk]

Some widget so we could test the main use case would be decent too

[aurelienlt]

Hi I created a small widget to test the new call:
https://github.com/aurelienlt/Beyond-All-Reason/blob/debug/test-build/luaui/Widgets/test_build.lua
By pressing "B" when a build command is selected, you switch to the widget, which let your dump some buildings, similarily to my GIF. This way also can see that the normal build mode still works fine.

[aurelienlt]

I've added some checks and fixes to avoid crashing with unexpected arguments. Surprisingly, I've found out that the engine doesn't enforce a building to follow the grid, so GiveOrderToUnit let your build in weird places. I haven't added code to enforce a valid building position, during my tests seems like grid is snapped to the right place though, probably because of int(testPos.x / SQUARE_SIZE) in CGameHelper::BuildSquareStatus.

[sprunk]

1. Squares fail to update if the blockedness state changes.
   

2. The "cacheValidity" arg doesn't seem to affect behaviour in any way.

[aurelienlt]

@sprunk So the current behavior is to never recompute the squares, and keep them in cache when removed according to cacheValidity. Mostly because I wanted to avoid having to recompute too many squares at once whereas they are only used for something very temporary, so I assumed players wouldn't be annoyed by corner cases.

I'm not sure what would be the best behavior, I'm open to suggestions. We could recompute them regularily once validity is passed despite being still used, we could add another argument for setting a refresh time ...

Also I've tested more thoroughly cacheValidity and fixed it.