Fixed TODO.
Details
- Apply the patch and compile the game
- Test building snapping feature for the test map
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 12871 Build 25224: Vulcan Build Jenkins Build 25223: Vulcan Build (macOS) Jenkins Build 25222: Vulcan Build (Windows) Jenkins
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2955/display/redirect
I do have trouble with e.g. the theatre, but that was present before.
Else snapping to the big faces works.
Nice addition. I only reviewed the math/code, didn't test yet.
source/simulation2/scripting/JSInterface_Simulation.cpp | ||
---|---|---|
44 | This function should go to source/simulation2/helpers/Geometry.{h,cpp}. There might be some slight duplication with DistanceToSquare. | |
57–58 | I understand the formula but this comment is not clear to me. We don't calculate a distance by doing this dot product. What would you think of We project the point, point A, and point B upon the direction of the segment to figure out in which space the point is. |
I neither fully understand the math, nor the code. I was merely answering to the call for testing ;)
No problem! It is just that I think a review of this kind of diffs need more of a code review than "I tested, it works -> accept." ;)
source/simulation2/scripting/JSInterface_Simulation.cpp | ||
---|---|---|
44 | Seems reasonable. | |
57–58 | Actually we do calculate distance, but not normalized. Since dot with a vector calculates C component in 2D line equation Ax + By + C = 0, where C is distance in (A, B) vectors from point (0, 0) to the input one. But I don't mind to change the comment to a more clear one. |
That I cannot place the theatre to the south-east or north-west facings of the wonder (small sides) because it snaps and I guess the obstructions overlap.
Successful build - Chance fights ever on the side of the prudent.
builderr-debug-macos.txt ../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces] CFixedVector2D(-halfSize.X, -halfSize.Y), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. builderr-release-macos.txt ../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces] CFixedVector2D(-halfSize.X, -halfSize.Y), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1332/display/redirect
Successful build - Chance fights ever on the side of the prudent.
builderr-debug-macos.txt ../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces] CFixedVector2D(-halfSize.X, -halfSize.Y), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. builderr-release-macos.txt ../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces] CFixedVector2D(-halfSize.X, -halfSize.Y), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1333/display/redirect
The code is good and it fixes a TODO. I also tested it in game and the snapping along the sides now work ??
As Freagarach mentioned, the fixed code makes apparent an issue with the obstruction of the Athenian wonder. There must be a mismatch between the obstruction and the size of the building.
I also remarked that building snapping breaks continuation of walls (placing a new section doesn't snap when general building snapping is activated). I don't know if this is a known issue. It's not related to this patch anyway.