HomeWildfire Games

Allow units to be positioned with freehand placement
AuditedrP21378

Description

Allow units to be positioned with freehand placement

Patch By: OptimusShepard
Comments By and Agreed With: elexis
Comments By: Imarok
Differential Revision: D1021
fixes #4791

Details

Auditors
temple
Imarok
OptimusShepard
Committed
bbFeb 25 2018, 11:17 PM
Differential Revision
D1021: Position units on a free drawn line
Parents
rP21377: Lower Nubia random map, refs #5040.
Branches
Unknown
Tags
Unknown
Build Status
Buildable 5224
Build 8914: Post-Commit BuildJenkins

Event Timeline

temple raised a concern with this commit.Feb 26 2018, 1:36 AM
temple added a subscriber: temple.
temple added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1187
WARNING: JavaScript warning: gui/session/input.js line 1187 reference to undefined property GetEntityState(...).unitAI

need a !!

This commit now has outstanding concerns.Feb 26 2018, 1:36 AM
bb requested verification of this commit.Feb 26 2018, 6:42 PM
This commit now requires verification by auditors.Feb 26 2018, 6:42 PM
temple accepted this commit.Feb 26 2018, 8:03 PM
All concerns with this commit have now been addressed.Feb 26 2018, 8:03 PM
elexis added a subscriber: elexis.Feb 27 2018, 8:50 PM
WARNING: JavaScript warning: Script value conversion check failed: v.isBoolean() (got type undefined)

Reproduce: have nothing selected, hold alt, right click drag
Fix: incoming

@OptimusShepard, @Imarok that we have the feature is good, but I suspect noone will know about it.

Adding a new button in the unit action panels could be sufficient to inform players.

But there is not enough space, is that correct?

(We could also mention it in the Tutorial, but that would only affect tutorial players, not necessarily the majority of players)

@OptimusShepard, @Imarok that we have the feature is good, but I suspect noone will know about it.
Adding a new button in the unit action panels could be sufficient to inform players.
But there is not enough space, is that correct?
(We could also mention it in the Tutorial, but that would only affect tutorial players, not necessarily the majority of players)

Maybe we need two different tutorials. One for beginners and one advanced tutorial which is useful for older players too.
Also we can show the release trailer (not skippable) when you start the game the first time. After first start you are be able to deselect this trailer. Such an trailer could be interesting for every next coming alpha, so this trailers sould be reselecting every new alpha (installing).
Also we need a discription on the homepage like the hotkeys.

OptimusShepard raised a concern with this commit.Oct 14 2018, 8:19 PM
OptimusShepard added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

This has to be entityDistribution = entityDistribution.slice(0, selection.length), or entityDistribution.splice(selection.length, entityDistribution.length - selection.length)

This commit now has outstanding concerns.Oct 14 2018, 8:19 PM
Stan added subscribers: Itms, Stan.Oct 14 2018, 10:18 PM
Stan added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

@elexis @Itms Can this be fixed before the re release ?

elexis added inline comments.Oct 16 2018, 6:40 PM
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

May we start with the examination before the conclusion?
Optimus can you describe for people too lazy to read the entire function how this bug can be observed within a game and how the correctness of your proposed fix is demonstrated without overcoming the burden of being lazy and deriving that from the code?

/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

Because of rounding errors the array of target positions could be too long or too short. In this code line this array should be cutten if the array is too long. The proble is that "slice" doesn't change the array. So this line of code is out of function. To solve this there are two possebilities:

  1. entityDistribution = entityDistribution.slice(0, selection.length)
  2. entityDistribution.splice(selection.length, entityDistribution.length - selection.length)

I didn't observe a problem until yet, because normaly the array lenght is correct or to short. But a too long array would currently cause an error in line 1217 because of an negativ array lenght.

bb added inline comments.Oct 17 2018, 7:58 PM
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

This line and the line below try to do exactly that (first make it not too long, then not too short), but indeed fail in doing the first (the first line currently doesn't do anything...), the first solution is to my memory the intended solution

bb marked an inline comment as done.Dec 26 2018, 5:02 PM
bb added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1216

Anyone complaining changing the line to entityDistribution = entityDistribution.slice(0, selection.length); My previous comment is pretty much the motivation for it

bb requested verification of this commit.Jan 5 2019, 9:47 PM

should be fixed now: rP22030

This commit now requires verification by auditors.Jan 5 2019, 9:47 PM
OptimusShepard accepted this commit.Jan 7 2019, 2:28 PM
All concerns with this commit have now been addressed.Jan 7 2019, 2:28 PM
Imarok raised a concern with this commit.Jan 18 2019, 1:29 PM
Imarok added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/input.js
1180

That sounds like it's wrong. !ev.button == SDL_BUTTON_RIGHT is equal to (!ev.button) == SDL_BUTTON_RIGHT and not to !(ev.button == SDL_BUTTON_RIGHT)

This commit now has outstanding concerns.Jan 18 2019, 1:29 PM

If someone had read the linting results of the patch....
(Yeah, I know input.js throws a bunch of warnings)

smiley raised a concern with this commit.Jan 20 2019, 11:04 AM
smiley added a subscriber: smiley.

I don't know whether this is an oversight or intended behavior that a Vector will be holding strings. If it's the latter, then it should not mutate the vector but return a string. Warrants a concern whichever one it may be IMO.
Also, the inline comment highlights the result of what the current code can do for someone caught off-guard.

/ps/trunk/binaries/data/mods/public/globalscripts/vector.js
104

Missing cast.

js> load("vector.js")
js> var a = new Vector2D(1.5555555, 3.7681982).toFixed(3)
js> var b = new Vector2D(3, 5)
js> Vector2D.add(a, b)
({x:"1.5563", y:"3.7685"})
js> var c = new Vector2D(1.5, 2.5)
js> Vector2D.add(a, c)
({x:"1.5561.5", y:"3.7682.5"})

Instead of requesting changes to D1251, raising a concern here since strings don't belong in a Vector.

elexis added inline comments.Jan 20 2019, 4:26 PM
/ps/trunk/binaries/data/mods/public/globalscripts/vector.js
104

test_Vector.js then

/ps/trunk/binaries/data/mods/public/gui/session/input.js
1180
bb added a comment.Mar 16 2019, 3:36 PM

@smiley: what would you think a "toFixed" would do on a vector? What we wanted to achieve is reducing the size of the command.txt for positioning units, thus dropping the precision after the 3rd decimal (as who cares about that?). To avoid duplication for any further such request a toFixed was added in the vector file. Regarding your example: if you use toFixed you expect a string, thus + gets a certain definition, so I don't see anything wrong....

/ps/trunk/binaries/data/mods/public/gui/session/input.js
1180

That indeed bugs, luckely (or sadly) it doesn't happen while playing as most players don't press the left mouse button while positioning units...

smiley claimed to have deleted his password, so I guess he can't remove himself from the subscription list now.

But I agree that it is unexpected that Vector2D.toFixed returns a string and not a number, because all the other methods are math operations with real numbers.

I think we can accomplish the commands.txt minimization without actually using strings? If it writes 2.5 to the file instead of "2.5", then the file actually gets few bytes smaller even.

Imarok accepted this commit.Mar 20 2019, 11:34 AM

My concern was fixed. Thanks.

smiley removed an auditor: smiley.Mar 30 2019, 9:35 PM

so I don't see anything wrong....

Ok.

All concerns with this commit have now been addressed.Mar 30 2019, 9:35 PM