Page MenuHomeWildfire Games
Differential D1415

Clean foundation sheep deletion and fix foundation mirages blocking the construction following D21
ClosedPublic
Actions

Authored by elexis on Mar 24 2018, 3:54 PM.

Details

Reviewers
temple
Commits
rP21624: Fix mirages (and any other entity that blocks something but not…
Trac Tickets
#4268
Summary

See discussion at rP21597.
The Animal hardcoding seems bad to me, adding a new flag much cleaner.

Test Plan

Should we queue the Entity deletion in C++ already? (It would remove the freedom to do something else in JS with the entity IDs).

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

elexis created this revision.Mar 24 2018, 3:54 PM
Vulcan added a subscriber: Vulcan.Mar 24 2018, 4:25 PM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 109| 109| 	var cmpPosition = Engine.QueryInterface(ent, IID_Position);
| 110| 110| 	var unitAI = Engine.QueryInterface(ent, IID_UnitAI);
| 111| 111| 	if (cmpPosition && !cmpPosition.IsInWorld() && unitAI && unitAI.IsGarrisoned())
| 112|    |-	{
|    | 112|+	
| 113| 113| 		// We're a garrisoned unit, assume impossibility as I've been unable to find a way to get the holder ID.
| 114| 114| 		// TODO: change this if that ever becomes possibles
| 115| 115| 		return false;
| 116|    |-	}
|    | 116|+	
| 117| 117| 	return true;
| 118| 118| }
| 119| 119| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 169| 169| 
| 170| 170| 	// Check if units are blocking our template change
| 171| 171| 	if (template.Obstruction && newTemplate.Obstruction)
| 172|    |-	{
|    | 172|+	
| 173| 173| 		// This only needs to be done if the new template is strictly bigger than the old one
| 174| 174| 		// "Obstructions" are annoying to test so just check.
| 175| 175| 		if (newTemplate.Obstruction.Obstructions ||
| 199| 199| 					return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, true);
| 200| 200| 			}
| 201| 201| 		}
| 202|    |-	}
|    | 202|+	
| 203| 203| 
| 204| 204| 	return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, false);
| 205| 205| }

binaries/data/mods/public/simulation/helpers/Transform.js
| 194| »   »   »   »   for·(let·ent·of·cmpNewObstruction.GetEntitiesDeletedUponConstruction())
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Transform.js
| 194| »   »   »   »   for·(let·ent·of·cmpNewObstruction.GetEntitiesDeletedUponConstruction())
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
|  47| »   let·pos·=·new·Vector2D(4,·5);
|    | [NORMAL] JSHintBear:
|    | 'pos' was used before it was defined.

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
| 121| »   let·cmpFoundation·=·ConstructComponent(foundationEnt,·"Foundation",·{});
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundation' was used before it was defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 321| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 323| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

Link to build: https://jenkins.wildfiregames.com/job/differential/308/display/redirect

Harbormaster failed remote builds in B5686: Diff 6269!Mar 24 2018, 4:25 PM
temple added a comment.Mar 24 2018, 5:34 PM

First test game had a gate that wouldn't close (once it was first opened) because there was a tree inside it. That's definitely a big bug, so we should either fix that or wait on the gate change.

binaries/data/mods/public/simulation/components/Foundation.js
197–198 ↗(On Diff #6269)

Hmm. For the original problem of animal corpses, if the structure doesn't block movement (e.g. a field) then we don't need to delete the corpse because we can still gather from it. So perhaps this should go in the if below.

binaries/data/mods/public/simulation/components/Gate.js
145 ↗(On Diff #6269)

let? if you want

146 ↗(On Diff #6269)

I guess GetBlockMovementFlag too, although the point of gates is that they block movement so why would it ever be false?

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
65 ↗(On Diff #6269)

GetEntitiesDeletedUponConstruction too

binaries/data/mods/public/simulation/helpers/Transform.js
194–195 ↗(On Diff #6269)

See, here we only do this if it blocks movement.

elexis marked 5 inline comments as done.Mar 25 2018, 3:22 PM

First test game had a gate that wouldn't close (once it was first opened) because there was a tree inside it. That's definitely a big bug, so we should either fix that or wait on the gate change.

While it is true that we shall not add things that players may perceive as regressions, I'd rather have working Foundation code now than later.
The tree shouldn't be placed into the iberian walls to begin with, so it's not the fault of the Foundation code.
To prevent the perceived regression, Ive added the Deletion flag to flora obstructions. It could happen with mines, but the likelihood is astronomically less than with trees (I've never seen it, maybe the distances to players are large enough already.)
I've marked the workaround as such and mentioned it in the place where the bug should be fixed, so that the one fixing the bug doesn't forget removing it.
Thanks for pointing out these two edge cases.

binaries/data/mods/public/simulation/components/Foundation.js
197–198 ↗(On Diff #6269)

Good point

220 ↗(On Diff #6269)

I guess this can remain, although Im not sure if it adds something (we could have so many TODOs if we have this one)

binaries/data/mods/public/simulation/components/Gate.js
146 ↗(On Diff #6269)

Maybe some sort of symbolic gate :-) Going for consistency

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
65 ↗(On Diff #6269)

oups

binaries/data/mods/public/simulation/helpers/Transform.js
194–195 ↗(On Diff #6269)

But you agree to copypaste this hunk in every occurence for the sake of being able to do something else/additional with the entityIDs?
(Could move both condition and deletion to cpp)

elexis marked 4 inline comments as done.Mar 25 2018, 3:26 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Foundation.js
197–198 ↗(On Diff #6269)

It's not 100% of the truth however, as it's not only sheep corpses, but there could be other obstructions that can't be removed through player actions. But given that we don't have any use case other than carcasses, I'll add the check.

elexis updated this revision to Diff 6272.Mar 25 2018, 3:27 PM

Only delete obstructions flagged for deletion that intersect the current obstruction when the current obstruction prevents movement.
Flag trees for deletion as random maps like to place them inside gates.
Add missing test thing.

Vulcan added a comment.Mar 25 2018, 3:40 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
|  47| »   let·pos·=·new·Vector2D(4,·5);
|    | [NORMAL] JSHintBear:
|    | 'pos' was used before it was defined.

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
| 122| »   let·cmpFoundation·=·ConstructComponent(foundationEnt,·"Foundation",·{});
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundation' was used before it was defined.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 109| 109| 	var cmpPosition = Engine.QueryInterface(ent, IID_Position);
| 110| 110| 	var unitAI = Engine.QueryInterface(ent, IID_UnitAI);
| 111| 111| 	if (cmpPosition && !cmpPosition.IsInWorld() && unitAI && unitAI.IsGarrisoned())
| 112|    |-	{
|    | 112|+	
| 113| 113| 		// We're a garrisoned unit, assume impossibility as I've been unable to find a way to get the holder ID.
| 114| 114| 		// TODO: change this if that ever becomes possibles
| 115| 115| 		return false;
| 116|    |-	}
|    | 116|+	
| 117| 117| 	return true;
| 118| 118| }
| 119| 119| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 169| 169| 
| 170| 170| 	// Check if units are blocking our template change
| 171| 171| 	if (template.Obstruction && newTemplate.Obstruction)
| 172|    |-	{
|    | 172|+	
| 173| 173| 		// This only needs to be done if the new template is strictly bigger than the old one
| 174| 174| 		// "Obstructions" are annoying to test so just check.
| 175| 175| 		if (newTemplate.Obstruction.Obstructions ||
| 199| 199| 					return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, true);
| 200| 200| 			}
| 201| 201| 		}
| 202|    |-	}
|    | 202|+	
| 203| 203| 
| 204| 204| 	return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, false);
| 205| 205| }

binaries/data/mods/public/simulation/helpers/Transform.js
| 194| »   »   »   »   for·(let·ent·of·cmpNewObstruction.GetEntitiesDeletedUponConstruction())
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Transform.js
| 194| »   »   »   »   for·(let·ent·of·cmpNewObstruction.GetEntitiesDeletedUponConstruction())
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 322| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 324| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

Link to build: https://jenkins.wildfiregames.com/job/differential/309/display/redirect

Harbormaster completed remote builds in B5690: Diff 6272.Mar 25 2018, 3:40 PM
Stan added a subscriber: Stan.Mar 25 2018, 4:31 PM

Mmmh does this patch also deletes actors or only templates ? In other words if you place this on grass will it delete it or not ?

Might want to notify mods that might break something if they have local copies of any of those root files

binaries/data/mods/public/maps/random/rmgen-common/player.js
125 ↗(On Diff #6272)

How hard is it to fix ?

elexis added a comment.Mar 25 2018, 4:36 PM
In D1415#58064, @Stan wrote:

Mmmh does this patch also deletes actors or only templates ? In other words if you place this on grass will it delete it or not ?

Only entities that have an Obstruction with the delete flag

Might want to notify mods that might break something if they have local copies of any of those root files

Urgh, why does DE copy that. Indeed (they will be notified by the errors however)

temple accepted this revision.Mar 25 2018, 7:51 PM

I have seen mines inside walls, but not recently, so just trees is okay.
Reads and tests fine, fixes the mirage problem from the previous commit.

binaries/data/mods/public/maps/random/rmgen-common/player.js
125 ↗(On Diff #6272)

trees inside walls

binaries/data/mods/public/simulation/components/Gate.js
223 ↗(On Diff #6272)

This is going to include trees now too, so gates won't close while a tree is there (although on map start they start closed). You have to lock the gate to delete the tree.

The bug is with the wall creation, and since players can still close the gate if they lock it (thanks to the tree template workaround), I think this is okay.

binaries/data/mods/public/simulation/helpers/Transform.js
194–195 ↗(On Diff #6269)

I don't have a preference either way.

This revision is now accepted and ready to land.Mar 25 2018, 7:51 PM
elexis added a comment.Mar 26 2018, 5:06 PM

Notice that the GetEntityCollisions change doesn't return units anymore that block things other than construction.
But that is a bugfix, since units that have DisableBlockMovement or DisableBlockPathfinding true for some reason shouldn't be returned either by definition.

There were some unused otherwise broken templates around that I have deleted in rP21621, rP21622 and rP21623 instead of updated here.
I would have said preview.xml should have it false and corpse.xml true and presented it as a good reason to have a flag. But since those have active false, they are excluded from the result anyhow.

Thanks for the review and help temple!

Closed by commit rP21624: Fix mirages (and any other entity that blocks something but not… (authored by elexis). · Explain WhyMar 26 2018, 5:19 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Mar 26 2018, 5:19 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
maps/
random/
rmgen-common/
3 lines
simulation/
components/
19 lines
15 lines
tests/
3 lines
helpers/
6 lines
templates/
special/
filter/
1 line
1 line
3 lines
1 line
2 lines
1 line
1 line
source/
simulation2/
components/
38 lines
19 lines
4 lines
11 lines

Diff 6276

View Options

ps/trunk/binaries/data/mods/public/maps/random/rmgen-common/player.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/Foundation.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/Gate.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Foundation.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/helpers/Transform.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/special/filter/mirage.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/special/filter/preview.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/special/filter/resource.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_gaia.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_gaia_flora.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit.xml

Loading...
View Options

ps/trunk/source/simulation2/components/CCmpObstruction.cpp

Loading...
View Options

ps/trunk/source/simulation2/components/ICmpObstruction.h

Loading...
View Options

ps/trunk/source/simulation2/components/ICmpObstruction.cpp

Loading...
View Options

ps/trunk/source/simulation2/components/ICmpObstructionManager.h

Loading...
Wildfire Games · Phabricator