Page MenuHomeWildfire Games

Decay/regenerate option for resources when not being gathered from
Changes PlannedPublic

Authored by Stan on Dec 30 2018, 9:17 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#1973
Summary

Salvaged from an old diff. Which was probably made after seeing a post on the forum requesting this feature for mods.
Also included is a use case for huntable animals which would rot away or be scavenged.

Test Plan

Agree this could be a nice addition.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7979
Build 12986: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan added inline comments.Feb 24 2019, 4:35 PM
binaries/data/mods/public/simulation/components/ResourceSupply.js
305

No because it's capped by the setAmount function.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  90|  90| 	if (this.template.Change && !this.infinite)
|  91|  91| 	{
|  92|  92| 		for (let changeKey in this.template.Change)
|  93|    |-		{
|    |  93|+		
|  94|  94| 			// Set the max amount a resource supply can reach to the highest limit defined.
|  95|  95| 			if (this.template.Change[changeKey].Limit !== undefined && this.template.Change[changeKey].Value > 0)
|  96|  96| 				this.maxAmount = Math.max(this.amount, +this.template.Change[changeKey].Limit);
|  97|    |-		}
|    |  97|+		
|  98|  98| 
|  99|  99| 		for (let changeKey in this.template.Change)
| 100| 100| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  97|  97| 		}
|  98|  98| 
|  99|  99| 		for (let changeKey in this.template.Change)
| 100|    |-		{
|    | 100|+		
| 101| 101| 			this.AddTimer(changeKey);
| 102|    |-		}
|    | 102|+		
| 103| 103| 	}
| 104| 104| 
| 105| 105| 	// List of IDs for each player
Executing section cli...

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

Stan updated this revision to Diff 7500.Feb 24 2019, 5:21 PM
  • Vulkan comments

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  90|  90| 	if (this.template.Change && !this.infinite)
|  91|  91| 	{
|  92|  92| 		for (let changeKey in this.template.Change)
|  93|    |-		{
|    |  93|+		
|  94|  94| 			// Set the max amount a resource supply can reach to the highest limit defined.
|  95|  95| 			if (this.template.Change[changeKey].Limit !== undefined && this.template.Change[changeKey].Value > 0)
|  96|  96| 				this.maxAmount = Math.max(this.amount, +this.template.Change[changeKey].Limit);
|  97|    |-		}
|    |  97|+		
|  98|  98| 
|  99|  99| 		for (let changeKey in this.template.Change)
| 100| 100| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  97|  97| 		}
|  98|  98| 
|  99|  99| 		for (let changeKey in this.template.Change)
| 100|    |-		{
|    | 100|+		
| 101| 101| 			this.AddTimer(changeKey);
| 102|    |-		}
|    | 102|+		
| 103| 103| 	}
| 104| 104| 
| 105| 105| 	// List of IDs for each player
Executing section cli...

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

Stan updated this revision to Diff 7504.Feb 24 2019, 10:21 PM

Upload the right diff.

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

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

Stan updated this revision to Diff 7506.Feb 25 2019, 9:54 AM
  • Remove delay as it will confusing for modders and there is no clean way to implement it without running into the previous problems such as constant reinits. Using timeouts the interval will not be too spammy.
Imarok updated the Trac tickets for this revision.Mar 10 2019, 6:58 PM
Imarok added a subscriber: Imarok.

I guess you also want to add tests for that new feature :)

Stan added a comment.Mar 11 2019, 11:21 AM
In D1718#72755, @Imarok wrote:

I guess you also want to add tests for that new feature :)

Will do once the main patch is accepted. :)

Stan updated this revision to Diff 7541.Mar 16 2019, 6:27 PM

Rebase following D1771 and rP22103

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

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

Stan marked an inline comment as done.Apr 19 2019, 6:17 PM

If I read this correctly, the constraint whether an entity is worked is removed because it is unrealistic that a dead animal decays slower when attended. But shouldn't the option at least be given? For it could be quite useful when considering farms: attended by workers means it grows (quickly), essentially making it an infinite supply, but when no workers are working it it decays. (This would make prolonged raiding a real pain.) This would probably require the generically named "constraint" to be renamed to something a bit more specific, e.g. "livingStatus".

Stan added a comment.Apr 27 2019, 9:59 PM

That would actually be a decay when not gathered, and grow when gathered, so two new options.

In D1718#76477, @Stan wrote:

That would actually be a decay when not gathered, and grow when gathered, so two new options.

Well, applied in the same way as the dead or alive option, so it could be used both ways. Perhaps someone would like to have something grow when not gathered, or stop decaying when gathered (like your initial idea on the meat not getting eaten by wild animals when someone is gathering it).
My initial hacky way of implementing this was:

	if (change.WorkedOrIdle &&
	    (change.WorkedOrIdle == "Worked" && this.GetNumGatherers() == 0 ||
	    change.WorkedOrIdle == "Idle" && this.GetNumGatherers() != 0))
		return;

in the apply change function. That, however, has a consequence that if a unit is tasked to the resource from the other side of the map, it is marked as being worked all the time so this is not the right solution.
When assuming that an empty resource will removed perhaps something can be done using the takeResources since that implies it is being worked?

Stan added a comment.Apr 28 2019, 12:16 PM

Well, applied in the same way as the dead or alive option, so it could be used both ways. Perhaps someone would like to have something grow when not gathered, or stop decaying when gathered (like your initial idea on the meat not getting eaten by wild animals when someone is gathering it).
My initial hacky way of implementing this was:

	if (change.WorkedOrIdle &&
	    (change.WorkedOrIdle == "Worked" && this.GetNumGatherers() == 0 ||
	    change.WorkedOrIdle == "Idle" && this.GetNumGatherers() != 0))
		return;

in the apply change function. That, however, has a consequence that if a unit is tasked to the resource from the other side of the map, it is marked as being worked all the time so this is not the right solution.
When assuming that an empty resource will removed perhaps something can be done using the takeResources since that implies it is being worked?

One could add a timer to know what was the last time TakeResources() was called, and only start decaying, or regrowing when it's been a certain amount of time. But I think that will add a lot of complexity for no benefit...

In D1718#76526, @Stan wrote:

One could add a timer to know what was the last time TakeResources() was called, and only start decaying, or regrowing when it's been a certain amount of time. But I think that will add a lot of complexity for no benefit...

In the TakeResources() reset the timer when "change.WorkedOrIdle == "Idle" " but I have no simple idea how to implement the Worked state ;)

Stan updated this revision to Diff 8415.EditedJun 10 2019, 3:42 PM
  • Add back decaying when not gathered as @Freagarach asked.
  • Fix a few bugs noticed by the unit tests I just added.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 256| 256| 	// Broadcast message, mainly useful for the AIs.
| 257| 257| 	Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() });
| 258| 258| 	if (this.GetNumGatherers() == 0 && this.template.Change && !this.IsInfinite())
| 259|    |-	{
|    | 259|+	
| 260| 260| 		for (let changeKey in this.template.Change)
| 261| 261| 			if (this.template.Change[changeKey].Constraint == "NotBeingGathered")
| 262| 262| 				this.AddTimer(changeKey);
| 263|    |-	}
|    | 263|+	
| 264| 264| };
| 265| 265| 
| 266| 266| /**
Executing section cli...

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

Questions:

  • This seems to allow only one constrain at a time. I think we might want to allow "Alive - Not Being Gathered" and "Dead - Not Being Gathered" to be more different than that for example.
  • Why do we allow decay when Alive? If we removed that, we could simplify the template considerably.
  • Naming of "Value", "Interval" and "Limit" isn't obvious imo. Also whether these are seconds, HP, or milliseconds?
Stan added a comment.Jun 12 2019, 10:23 PM

Questions:

  • This seems to allow only one constrain at a time. I think we might want to allow "Alive - Not Being Gathered" and "Dead - Not Being Gathered" to be more different than that for example.

See the test, you can combine them all :)

  • Why do we allow decay when Alive? If we removed that, we could simplify the template considerably.

It's anyname, so I could have called it growth :) I just gave them explicit names

  • Naming of "Value", "Interval" and "Limit" isn't obvious imo. Also whether these are seconds, HP, or milliseconds?

Value → Is the amount per second I think I copied the names from the health component
Limit is also a number of resource
Interval is Milisec :)

In D1718#82138, @Stan wrote:

See the test, you can combine them all :)

I don't see a test with multiple constraint in the same element?

It's anyname, so I could have called it growth :) I just gave them explicit names

I meant why is there a constraint for "Alive".

Value → Is the amount per second I think I copied the names from the health component
Limit is also a number of resource
Interval is Milisec :)

It's obviously not the amount per second if you have an interval ;)

I would prefer perhaps there interval be removed and Value be renamed "Rate". Is Limit a threshold? A maximum from the start? I think the names can be improved.

Stan added a comment.Jun 12 2019, 10:32 PM
In D1718#82138, @Stan wrote:

See the test, you can combine them all :)

I don't see a test with multiple constraint in the same element?

Ah yes it doesn't work like that you just add as many elements as you need, and they work together depending on the constraint

So you could have growth , regen, and another doing stuff when the resource alive

And decay, rotting, and burning doing something else when the resource is dead

You can also have the no gatherer decay or grow the resource all at the same time.

It's anyname, so I could have called it growth :) I just gave them explicit names

I meant why is there a constraint for "Alive".

Cause, trees are not alive, sheeps are.

Value → Is the amount per second I think I copied the names from the health component
Limit is also a number of resource
Interval is Milisec :)

It's obviously not the amount per second if you have an interval ;)
I would prefer perhaps there interval be removed

People asked for it, in the huge amount of comments before... there are a lot of discussions above.

and Value be renamed "Rate".

This was discussed above as well I believe.

Is Limit a threshold? A maximum from the start? I think the names can be improved.

It can be both, hence limit.

In D1718#82140, @Stan wrote:

So you could have growth , regen, and another doing stuff when the resource alive
And decay, rotting, and burning doing something else when the resource is dead
You can also have the no gatherer decay or grow the resource all at the same time.

Mh, I would say no, unless I've misunderstood something. You can have a constraint for "Alive", a constraint for "Dead", and one for "NotBeingGathered". That can give you up to 4 different rates: Alive / Alive + NotBeing Gathered + Dead / Dead + NotBeing Gathered, but actually Alive / Dead + NotBeingGathered would be tied somewhat since you can't have a different value to NotBeingGathered for both, which seems impractical.

I find myself agreeing with @Nescio above:

  • remove the AnyName business and only have two possible elements "ChangeWhenAlive", "ChangeWhenDead" or something. I doubt many items would have similar values for both, so the redundancy doesn't seem too bad.
  • Add a "BeingGathered" multiplier for the value of each of these. So then you actually have 4 values.

You told him

Also your solution makes applying techs impossible :) for instance an aura that would boost the growth.

I don't see why?

Cause, trees are not alive, sheeps are.

Ah, right, so this simulates fattening. Sorry, missed that.

It can be both, hence limit.

Mh, I'd still prefer something like MaxAmount

Stan added a comment.Jun 12 2019, 10:53 PM
In D1718#82140, @Stan wrote:

So you could have growth , regen, and another doing stuff when the resource alive
And decay, rotting, and burning doing something else when the resource is dead
You can also have the no gatherer decay or grow the resource all at the same time.

Mh, I would say no, unless I've misunderstood something. You can have a constraint for "Alive", a constraint for "Dead", and one for "NotBeingGathered". That can give you up to 4 different rates: Alive / Alive + NotBeing Gathered + Dead / Dead + NotBeing Gathered, but actually Alive / Dead + NotBeingGathered would be tied somewhat since you can't have a different value to NotBeingGathered for both, which seems impractical.

I can have Alive1 Alive2 Alive3 Alive4 :) Indeed NotBeingGathered could use an Alive and Dead State

I find myself agreeing with @Nescio above:

  • remove the AnyName business and only have two possible elements "ChangeWhenAlive", "ChangeWhenDead" or something. I doubt many items would have similar values for both, so the redundancy doesn't seem too bad.
  • Add a "BeingGathered" multiplier for the value of each of these. So then you actually have 4 values.

You told him

Also your solution makes applying techs impossible :) for instance an aura that would boost the growth.

I don't see why?

Cause imagine I have an aura boosting Growth but not Fattening because the person is a sheep herder (let's take aside the alive part), not a gardener

If I only have ChangeWhenAlive or change when dead, I can't make any difference between the two. Hence the aura would boost both which I do not want.

Cause, trees are not alive, sheeps are.

Ah, right, so this simulates fattening. Sorry, missed that.

It's okay.

It can be both, hence limit.

Mh, I'd still prefer something like MaxAmount

It would be lying when it caps minimum amount below which it does not decay :*

I can have Alive1 Alive2 Alive3 Alive4 :) Indeed NotBeingGathered could use an Alive and Dead State

I don't see how having multiple Alive would make any difference than just having one with the combined effects?

Cause imagine I have an aura boosting Growth but not Fattening because the person is a sheep herder (let's take aside the alive part), not a gardener
If I only have ChangeWhenAlive or change when dead, I can't make any difference between the two. Hence the aura would boost both which I do not want.

Couldn't you work around that with identity classes, rather? If not, I guess I amend the above and this should look like the following instead of having 'Constraint'

<Fattening>
	<Alive/>
	<NotBeingGathered/>
	<Value>5</Value>
	<Interval>34235</Interval>
</Fattening>

It would be lying when it caps minimum amount below which it does not decay :*

Er, "MaxChange" perhaps? Limit just sounds super vague to me, particularly with an Interval component which makes it sound like it's a limit in Time (which I guess it is... To some extent).

nani removed a subscriber: nani.Jun 12 2019, 11:03 PM
Stan added a comment.Jun 12 2019, 11:06 PM

I can have Alive1 Alive2 Alive3 Alive4 :) Indeed NotBeingGathered could use an Alive and Dead State

I don't see how having multiple Alive would make any difference than just having one with the combined effects?

Easier to deal with auras and whatnot.

Cause imagine I have an aura boosting Growth but not Fattening because the person is a sheep herder (let's take aside the alive part), not a gardener
If I only have ChangeWhenAlive or change when dead, I can't make any difference between the two. Hence the aura would boost both which I do not want.

Couldn't you work around that with identity classes, rather? If not, I guess I amend the above and this should look like the following instead of having 'Constraint'

<Fattening>
	<Alive/>
	<NotBeingGathered/>
	<Value>5</Value>
	<Interval>34235</Interval>
</Fattening>

Do you have an example ? I'm not sure it would help though.

I'm okay with replacing constraints with tags.

It would be lying when it caps minimum amount below which it does not decay :*

Er, "MaxChange" perhaps? Limit just sounds super vague to me, particularly with an Interval component which makes it sound like it's a limit in Time (which I guess it is... To some extent).

I guess I could use Max Change or ChangeLimit maybe

Stan updated this revision to Diff 8459.Jun 13 2019, 9:32 PM
  • Add more tests.
  • Replace constraints with tags to allow mixing them
  • Rename Limit to changelimit

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 250| 250| 		for (let changeKey in this.template.Change)
| 251| 251| 			this.AddTimer(changeKey);
| 252| 252| 	}
| 253|    |-}
|    | 253|+};
| 254| 254| 
| 255| 255| /**
| 256| 256|  * @param {number} gathererID - The gatherer's entity id.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 276| 276| 	// Broadcast message, mainly useful for the AIs.
| 277| 277| 	Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() });
| 278| 278| 	if (this.GetNumGatherers() == 0 && this.template.Change && !this.IsInfinite())
| 279|    |-	{
|    | 279|+	
| 280| 280| 		for (let changeKey in this.template.Change)
| 281| 281| 			if (!!this.template.Change[changeKey].NotBeingGathered)
| 282| 282| 				this.AddTimer(changeKey);
| 283|    |-	}
|    | 283|+	
| 284| 284| };
| 285| 285| 
| 286| 286| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 318| 318| 	if (!change.NotBeingGathered)
| 319| 319| 	{
| 320| 320| 		if (!change.Alive || !change.Dead)
| 321|    |-		{
|    | 321|+		
| 322| 322| 			if (!!change.Alive && !cmpHealth || !!change.Dead && cmpHealth)
| 323| 323| 			{
| 324| 324| 				this.AddTimer(changeKey);
| 325| 325| 				return;
| 326| 326| 			}
| 327|    |-		}
|    | 327|+		
| 328| 328| 	}
| 329| 329| 	else
| 330| 330| 	{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 327| 327| 		}
| 328| 328| 	}
| 329| 329| 	else
| 330|    |-	{
|    | 330|+	
| 331| 331| 		if (!!change.Alive && !cmpHealth && !change.Dead || !!change.Dead && cmpHealth && !change.Alive)
| 332| 332| 		{
| 333| 333| 			this.AddTimer(changeKey);
| 334| 334| 			return;
| 335| 335| 		}
| 336|    |-	}
|    | 336|+	
| 337| 337| 
| 338| 338| 
| 339| 339| 	let newAmount = ApplyValueModificationsToEntity("ResourceSupply/Change/" + changeKey + "/Value", +change.Value, this.entity);

binaries/data/mods/public/simulation/components/ResourceSupply.js
| 253| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

Stan updated this revision to Diff 8462.Jun 13 2019, 11:15 PM
  • Tests are only reliable if well written, thanks @Freagarach
  • Simplify code a bit.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 242| 242| 
| 243| 243| 	for (let changeKey in this.template.Change)
| 244| 244| 		this.AddTimer(changeKey);
| 245|    |-}
|    | 245|+};
| 246| 246| 
| 247| 247| /**
| 248| 248|  * @param {number} gathererID - The gatherer's entity id.

binaries/data/mods/public/simulation/components/ResourceSupply.js
| 245| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|  89|  89| 	"KillBeforeGather": false,
|  90|  90| 	"Change": {
|  91|  91| 		"Rotting": {
|  92|    |-			"NotBeingGathered": void(0),
|    |  92|+			"NotBeingGathered": void (0),
|  93|  93| 			"Value": -1,
|  94|  94| 			"Interval": 500
|  95|  95| 		}
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 123| 123| 	"KillBeforeGather": true,
| 124| 124| 	"Change": {
| 125| 125| 		"Growth": {
| 126|    |-			"Alive": void(0),
|    | 126|+			"Alive": void (0),
| 127| 127| 			"Value": 5,
| 128| 128| 			"Interval": 500,
| 129| 129| 			"ChangeLimit": 1010
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 162| 162| 	"KillBeforeGather": {},
| 163| 163| 	"Change": {
| 164| 164| 		"Growth": {
| 165|    |-			"Alive": void(0),
|    | 165|+			"Alive": void (0),
| 166| 166| 			"Value": 5,
| 167| 167| 			"Interval": 500,
| 168| 168| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 168| 168| 			"ChangeLimit": 2000
| 169| 169| 		},
| 170| 170| 		"Decay": {
| 171|    |-			"Dead": void(0),
|    | 171|+			"Dead": void (0),
| 172| 172| 			"Value": -3,
| 173| 173| 			"Interval": 500,
| 174| 174| 			"ChangeLimit": 994
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 213| 213| 	"KillBeforeGather": true,
| 214| 214| 	"Change": {
| 215| 215| 		"Growth": {
| 216|    |-			"Alive": void(0),
|    | 216|+			"Alive": void (0),
| 217| 217| 			"Value": 5,
| 218| 218| 			"Interval": 500,
| 219| 219| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 219| 219| 			"ChangeLimit": 2000
| 220| 220| 		},
| 221| 221| 		"Decay": {
| 222|    |-			"Dead": void(0),
|    | 222|+			"Dead": void (0),
| 223| 223| 			"Value": -3,
| 224| 224| 			"Interval": 500,
| 225| 225| 			"ChangeLimit": 994
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 246| 246| 	"KillBeforeGather": true,
| 247| 247| 	"Change": {
| 248| 248| 		"Growth": {
| 249|    |-			"Alive": void(0),
|    | 249|+			"Alive": void (0),
| 250| 250| 			"Dead": void(0),
| 251| 251| 			"Value": 5,
| 252| 252| 			"Interval": 1000,
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 247| 247| 	"Change": {
| 248| 248| 		"Growth": {
| 249| 249| 			"Alive": void(0),
| 250|    |-			"Dead": void(0),
|    | 250|+			"Dead": void (0),
| 251| 251| 			"Value": 5,
| 252| 252| 			"Interval": 1000,
| 253| 253| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 280| 280| 	"KillBeforeGather": true,
| 281| 281| 	"Change": {
| 282| 282| 		"Growth": {
| 283|    |-			"Alive": void(0),
|    | 283|+			"Alive": void (0),
| 284| 284| 			"NotBeingGathered": void(0),
| 285| 285| 			"Value": 5,
| 286| 286| 			"Interval": 1000,
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 281| 281| 	"Change": {
| 282| 282| 		"Growth": {
| 283| 283| 			"Alive": void(0),
| 284|    |-			"NotBeingGathered": void(0),
|    | 284|+			"NotBeingGathered": void (0),
| 285| 285| 			"Value": 5,
| 286| 286| 			"Interval": 1000,
| 287| 287| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 313| 313| 	"KillBeforeGather": true,
| 314| 314| 	"Change": {
| 315| 315| 		"Growth": {
| 316|    |-			"Alive": void(0),
|    | 316|+			"Alive": void (0),
| 317| 317| 			"NotBeingGathered": void(0),
| 318| 318| 			"Value": 5,
| 319| 319| 			"Interval": 1000,
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 314| 314| 	"Change": {
| 315| 315| 		"Growth": {
| 316| 316| 			"Alive": void(0),
| 317|    |-			"NotBeingGathered": void(0),
|    | 317|+			"NotBeingGathered": void (0),
| 318| 318| 			"Value": 5,
| 319| 319| 			"Interval": 1000,
| 320| 320| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 320| 320| 			"ChangeLimit": 2000
| 321| 321| 		},
| 322| 322| 		"Regen": {
| 323|    |-			"NotBeingGathered": void(0),
|    | 323|+			"NotBeingGathered": void (0),
| 324| 324| 			"Value": 5,
| 325| 325| 			"Interval": 1000,
| 326| 326| 			"ChangeLimit": 2000
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 352| 352| 	"KillBeforeGather": true,
| 353| 353| 	"Change": {
| 354| 354| 		"Decay": {
| 355|    |-			"Dead": void(0),
|    | 355|+			"Dead": void (0),
| 356| 356| 			"NotBeingGathered": void(0),
| 357| 357| 			"Value": -5,
| 358| 358| 			"Interval": 1000,
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unary word operator 'void' must be followed by whitespace.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 353| 353| 	"Change": {
| 354| 354| 		"Decay": {
| 355| 355| 			"Dead": void(0),
| 356|    |-			"NotBeingGathered": void(0),
|    | 356|+			"NotBeingGathered": void (0),
| 357| 357| 			"Value": -5,
| 358| 358| 			"Interval": 1000,
| 359| 359| 			"ChangeLimit": 492
Executing section cli...

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

Stan updated this revision to Diff 8463.Jun 13 2019, 11:21 PM

Should make the linter happy.

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

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

In D1718#82219, @Stan wrote:
  • Tests are only reliable if well written, thanks @Freagarach
  • Simplify code a bit.

Very glad to be of service :) But where do I owe the honour of being thanked to? ;)

Stan added a comment.Jun 14 2019, 7:00 PM

If it wasn't for your reminder that !! Doesn't work with solo tags I wouldn't have noticed the flaw.

In D1718#82231, @Stan wrote:

If it wasn't for your reminder that !! Doesn't work with solo tags I wouldn't have noticed the flaw.

Ah okay, always good to accidentally help people :)

It seems to me like your tests could be abstracted in a function to remove a lot of redundancy.

Easier to deal with auras and whatnot.

OK, so I guess your position is that you want to enable having a "faster fattening" aura/tech that changes ResourceSupply/Fattening/Value, instead of having a tech that changes ResourceSupply/Alive/Value and only affects items with Identity classes Organic, Domestic for example. That's internally coherent, and I do agree that on the surface it seems more craftier - both systems actually allow the same thing but using identity classes might force one to add arbitrary identity classes to templates, which seems undesirable.
However this doesn't seem obvious from reading the code, nor the tests. I think you should be able to add an Aura in the tests that only affects Fattening, not say Growth, to highlight that.
I'm not sure where this would be best to document, perhaps in the schema as a a:help or as a comment?

Stan added a comment.Jun 15 2019, 12:17 PM

It seems to me like your tests could be abstracted in a function to remove a lot of redundancy.

Easier to deal with auras and whatnot.

OK, so I guess your position is that you want to enable having a "faster fattening" aura/tech that changes ResourceSupply/Fattening/Value, instead of having a tech that changes ResourceSupply/Alive/Value and only affects items with Identity classes Organic, Domestic for example. That's internally coherent, and I do agree that on the surface it seems more craftier - both systems actually allow the same thing but using identity classes might force one to add arbitrary identity classes to templates, which seems undesirable.
However this doesn't seem obvious from reading the code, nor the tests. I think you should be able to add an Aura in the tests that only affects Fattening, not say Growth, to highlight that.

Didn't we have that discussion already that it was particularly tricky to add auras during the tests ?

I'm not sure where this would be best to document, perhaps in the schema as a a:help or as a comment?

I will try to add functions for the tests, might make them more consistent..

Didn't we have that discussion already that it was particularly tricky to add auras during the tests ?

We did, that's why D274 exists - but I think auras are doable, whereas techs are extremely annoying to add. That being said, it's not a hard requirement from me, this can be merged without these imo.

Stan updated this revision to Diff 8473.Jun 15 2019, 12:55 PM
  • Use function for the tests to remove duplication.

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

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

binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|  92| function·TestChangeModifiers(template,·entity,·results)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|  92| function·TestChangeModifiers(template,·entity,·results)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'entity' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
|  94| »   let·cmpResourceSupply·=·ConstructComponent(entity,·"ResourceSupply",·template);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpResourceSupply' is already declared in the upper scope.
Executing section cli...

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

Stan updated this revision to Diff 8477.Jun 15 2019, 3:36 PM

Fix warnings

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

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

Stan updated this revision to Diff 8478.Jun 15 2019, 3:39 PM
  • Do not break the test in the process

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

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

Stan updated this revision to Diff 8523.Jun 16 2019, 8:45 PM
  • Add aura tests.

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

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

binaries/data/mods/public/simulation/components/tests/test_ResourceSupply.js
| 126| »   »   "ApplyModifications":·(key,·val,·ent)·=>·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.
Executing section cli...

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

Stan added a comment.Jun 27 2019, 10:24 AM

@wraitii Anything else you want me to change aside from the warning in Jenkins console ?

@Stan I haven't really looked deep into your patch, so perhaps, yes. I don't consider what I've said above a review.

Stan added a comment.Jun 27 2019, 10:37 AM

@Stan I haven't really looked deep into your patch, so perhaps, yes. I don't consider what I've said above a review.

Okay thanks for the heads up :)

It's been a long time since I last looked at this. If it's not too much trouble, perhaps you could explain how it works/syntax in xml templates?

Stan added a comment.Jun 27 2019, 11:02 AM

Well it might change again before it's committed but currently you can do something like

<Change>
    <AnyName> 
        <Alive/> 
        <Dead/>
        <NotBeingGathered/> 
        <Value>2</Value> 
        <Interval>1000</Interval> 
        <ChangeLimit>5000</ChangeLimit>
    </AnyName> 
</Change>

ChangeLimit defines the lowest or the highest the specific change can go if limit is higher than the max amount, the max amount is set to that value.
Interval defines how often the change is run.
Value (positive or negative) defines what will be done to the resource
Then you have three flags that can be combined or use individually.

If dead is present then the resource only decay/regrow when dead.
if alive is present then the resource only decay/regrow when alive.
When both are there, it always decay.
If NotBeingGathered is present it controls the two others by adding or cancelling the decay when gatherers are removed or added.

Thanks, that's helpful.
Alive and Dead are clear for fauna (fatten when living, decay when killed), but not really for gaia templates (is a tree considered alive or dead?).
Does NotBeingGathered means it will or won't work? Also, shouldn't there be a OnlyWhenBeingGathered tag then too?
Furthermore, there is probably a clearer term for ChangeLimit.

Stan added a comment.Jun 27 2019, 11:17 AM
In D1718#84088, @Nescio wrote:

Thanks, that's helpful.
Alive and Dead are clear for fauna (fatten when living, decay when killed), but not really for gaia templates (is a tree considered alive or dead?).

A tree is considered dead because it does not have health.

Does NotBeingGathered means it will or won't work? Also, shouldn't there be a OnlyWhenBeingGathered tag then too?

Alive + Notbeinggathered only works when alive and not being gathered
Dead + Notbeinggathered only works when dead and not being gathered
Alive + Dead + Notbeinggathered only works when not being gathered.
Notbeinggathered only works when not being gathered.

Furthermore, there is probably a clearer term for ChangeLimit.

We tried to find a word, but it's not a treshold, nor it is a maximum, nor it is a minium. Hence limit.

Alive + Dead + Notbeinggathered only works when not being gathered.

Why? Surely something can't be alive and dead simultaneously? (No Schrödinger's cat in 0 A.D.)
Also, what if someone wants a resource change that only works when being gathered (making something)?

Hence limit.

Then why ChangeLimit and not just Limit?

Stan added a comment.Jun 27 2019, 11:33 AM
In D1718#84090, @Nescio wrote:

Alive + Dead + Notbeinggathered only works when not being gathered.

Why? Surely something can't be alive and dead simultaneously? (No Schrödinger's cat in 0 A.D.)

Alive and dead means the change works always, not that the thing has to be both alive and dead. Alive and dead is equal to not having them at all.

Also, what if someone wants a resource change that only works when being gathered (making something)?

Well he is out of luck with the current implementation...

Hence limit.

Then why ChangeLimit and not just Limit?

Cause wraitii asked me to change it :P

Alive and dead is equal to not having them at all.

Isn't that confusing?

Stan added a comment.Jun 27 2019, 11:59 AM
In D1718#84092, @Nescio wrote:

Alive and dead is equal to not having them at all.

Isn't that confusing?

Well that's only because you have NotBeinGathered with the other two.

It's like saying the resource change when being alive, and when being dead, but only when not being gathered.
or saying the the resource change when not being gathered.
I would usually recommend people do multiple changes instead of combining them of the sort
One for the dead part, one for the alive part, which would gave them more control.

I guess "LimitToTheChange" or something longer would be clearer... This mechanic is very flexible, perhaps too much for its own good.
One question I will ask myself on this is whether status effects should be used for this - it seems like they might, with some changes - as @elexis noted. Which might end up reducing complexity, or at least genericing it usefully.

Stan added a comment.Jun 27 2019, 12:24 PM

Means rewriting the whole patch, but okay I guess...

This comment was removed by wraitii.
wraitii added a comment.EditedJun 27 2019, 2:10 PM

Bit of a more complete review:

There's a bunch of things going on here:

  • You're implementing ResourceSupply/Change/[Key]/Value modifier, which triggers a timer for changing the supply of a resource.
  • You're defining a few different Change possibilities.
  • You're triggering these Change possibilities based on constraints (alive, dead, notbeingGathered).
  • You are "untriggering" these based on the Change.

The third item is specific to resources. The "Alive/Dead" check seems like it is a workaround for us having no ability to transform components when applying a filter (since dead entities become corpse| or resource| variants). So we should fix that. We ran in the same problem with armour and its "foundation" sub-item.

The other three definitely look like status effects to me, and would benefit from being merged with that code. In particular, this would let us:

  • have GUI support (when we implement that for status effects)
  • have generic modifiers StatusEffect/[Key]/ResourceSupplyChange/Value
  • Implement limits in a cleaner fashion, using "MinValue", "MaxValue", "AmountBeforeExpiration" or something like that. This logic could be mad generic for status effects, since damage effects might want to have these too.

Currently StatusEffects implements damaging only. We could extend it to implement resource supply changes. The timer logic is already implemented there (perhaps must be improved) so that limits repetitions.

So my suggestion is to keep the triggering code, but to make it trigger a status effect, and move the rest of the logic to status effects. We might need to define a schema for status effects there.

At the moment, I would keep the definition of the status effects in the ResourceSupply template.

Edit: possibly resource trickles should also be implemented as status effects - it's a bit of a blurry line there to me -

Stan planned changes to this revision.EditedJun 27 2019, 2:48 PM

Well given the current state of things around here and the impact it has on me, I guess this won't happen soonish. Maybe I'll get to do it for A24 but I'm not sure anymore. 47th revision (192 comments)

I'm marking this as planned changes, but, feel free to commandeer.