Page MenuHomeWildfire Games

Implements correct distance to edge for building snapping
ClosedPublic

Authored by vladislavbelov on Aug 10 2020, 12:50 AM.

Details

Summary

Fixed TODO.

Test Plan
  1. Apply the patch and compile the game
  2. Test building snapping feature for the test map

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Mod with a test map with big buildings.

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

vladislavbelov requested review of this revision.Aug 10 2020, 1:10 AM

I do have trouble with e.g. the theatre, but that was present before.

Else snapping to the big faces works.

Itms requested changes to this revision.Aug 11 2020, 5:25 PM
Itms edited reviewers, added: Itms, Freagarach; removed: Restricted Owners Package.
Itms added a subscriber: Itms.

Nice addition. I only reviewed the math/code, didn't test yet.

source/simulation2/scripting/JSInterface_Simulation.cpp
43 ↗(On Diff #13154)

This function should go to source/simulation2/helpers/Geometry.{h,cpp}. There might be some slight duplication with DistanceToSquare.

56–57 ↗(On Diff #13154)

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.

This revision now requires changes to proceed.Aug 11 2020, 5:25 PM
Itms removed a reviewer: Restricted Owners Package.Aug 11 2020, 5:26 PM
Freagarach resigned from this revision.Aug 11 2020, 5:30 PM
Freagarach removed a reviewer: Freagarach.

I neither fully understand the math, nor the code. I was merely answering to the call for testing ;)

Itms added a comment.Aug 11 2020, 5:30 PM

Ha sorry! I really have to test it, then ?

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." ;)

vladislavbelov added inline comments.Aug 11 2020, 9:57 PM
source/simulation2/scripting/JSInterface_Simulation.cpp
43 ↗(On Diff #13154)

Seems reasonable.

56–57 ↗(On Diff #13154)

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.

I do have trouble with e.g. the theatre, but that was present before.

Which trouble?

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

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.

E.g.

Itms accepted this revision.Aug 16 2020, 8:08 PM

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.

This revision is now accepted and ready to land.Aug 16 2020, 8:08 PM
Owners added a subscriber: Restricted Owners Package.Aug 17 2020, 10:14 PM