Page MenuHomeWildfire Games

Decay/regenerate option for resources when not being gathered from
Needs ReviewPublic

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 6969
Build 11400: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan updated this revision to Diff 7494.Feb 19 2019, 11:38 AM
Stan marked 5 inline comments as done.
  • Fix space after xml tag
  • Add limit
  • Fix missing changes after cleanup
  • Make delay not optional
  • Exclude 0 from interval
  • Use changeKey instead of key.

Is this actually the first <anyName/>? I thought I saw it somewhere else, but if it's really the first occurrence, it might be a good indication to not start using it unless we have compelling evidence to use it.
All value strings that I find it tech and aura JSON files don't include custom names. (The closest we have are resource subtypes, and they are also not really arbitrary but fixed choice ResourceGatherer/Rates/food.grain)

No, It's used in Attack.js, DeathDamage.js, Formation.js, GarrisonHolder.js, Health.js, Sound.js.

Monsters = spooky, spooky = bad? Just fix up what we have in such a way that the changes for future adventures can be added without rewriting this one?

Kind of. Though monster = annoying patch to review and to make, = less chance that the game is worth the hassle, less chance that the player will notice it, more maintenance. Also it means that I'm not doing anything else because it kinda clouds my mind to be focused on such a thing.

I take it as a compliment that you ask me, but I'm already making an exemption to my inactivity here.

It's a compliment, even though I hate having to rewrite patches I'm glad you are taking time for me while you could be doing so much more for the game. You're one of the persons that makes me wish I was a better programmer.
When you say you are inactive do you mean on the game or on phab ?

I was merely mentionning it because you asked me to make a separate patch for it.

I could help with the templates, but not with the code, unfortunately.

Yeah templates will be fine though, it's just tedious work but it's not complicated. I just hate to break mods. Cause then I either feel entitled to or I just fix it.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1063/

elexis added inline comments.Feb 19 2019, 3:52 PM
binaries/data/mods/public/simulation/components/Health.js
300

(So was my suspicion that the rest of the code uses entFoo actually true or did we act on suspicion? I didn't search the code. Otherwise, the entFoo seems more informative than Foo)

binaries/data/mods/public/simulation/components/ResourceSupply.js
23
  • Limit should be demonstrated (or must be demonstrated if it becomes non-optiona)
  • I think most readers will think that the values must be Growth and Decay, at least until they read the part at the bottom. One could consider using a name that implies that the name can be chosen arbitrarily (assuming <anyName/> implementation). Growth and Decay are good though.
38

But thats the same as Limit, so there is no purpose for this field, not in this patch (nor in the random starting resources amount patch).
The only user for that GetMaxValue function is the status bar (SetAmount limiting looks duplicate)? So shouldn't the function return max(limit1, limit2, ..., template.amount) or something, possibly cached in a non-serialized variable? (the latter is easy, there are examples of sim components that replace Serialize and Deserialize, the Serialize function just returns an object with all values to be serialized (i.e. excluding the cahce variables) and the Deserialize function copies all these given object properties to this and reinitializes the cache variable). As there already are some cache variables, this could be done separately.

71
  • I guess you don't want to exclude zero here. :P
  • "change has no effect" sounds like a contradiction unless using Change (name) != change (common connotation). It could be named Effect or Generator, and that would become inactive if the limit is reached. But not my decision how to label it. It should be the most appealing solution to the readers and users.
  • If it's optional, then it can go below 0, which contradicts the positive nature of resource amounts.
181

Above mentioned duplicate limit check. Could pass min/max to this function or rely on the caller having set the limit already and setting here whatever is given. In case the latter is chosen, TakeResources would obviously have to use max(x - y, 0).

190–192

This function returns exhausedso that something else can check to remove the resource if it's exhaused?
The same thing needs to happen for decayed resources too?

275
  • limit undefined
  • ||\n
  • - looks wrong
  • Early return seems wrong, what if the new amount is 10 and the resource is at 95/100, it would not go to 100?
318

That check can be more strict, elements[0], elements[1] or so are expected to have a specific value too, no?

325

Delay thing I mentioned, I didn't investigate how often in other code timers are restarted if a local aura appeared, so maybe updating the interval only if the current interval ran out isn't better.

Recap: Imagine an aura entity running in and out of aura range of the corpses and the corpses restart their interval each time that happens. If the timer is restarted more often than the interval, then the corpses won't decay anymore at all, even if the aura would only reduce it by < 100%.

Stan updated this revision to Diff 7495.Feb 19 2019, 5:04 PM
Stan marked 10 inline comments as done.
Stan added inline comments.
binaries/data/mods/public/simulation/components/Health.js
300

I didn't check but the request sounded fair.

binaries/data/mods/public/simulation/components/ResourceSupply.js
38

No it's not the same.

Max Amount 5000
Amount 4000
Limit 4500

Bar displays x/5000
Regen goes up to 4500/5000

Max Amount 5000
Amount 4000

Bar displays x/5000
Regen goes up to 5000/5000

Amount 4000

Bar displays x/4000
Regen goes up to 4000/4000

71

Not it cannot because setAmount won't let it.

181

I don't understand. It's capped so it doesn't break the UI and it's destroyed when it reaches 0. Every code setting the amount should go through set amount. Also we pass the old amount not the new one.

190–192

Something else is whatever is calling it so IIRC Petra and UnitAI which means that in any case once the number has reached 0 the resource is exhausted.

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
| 258| 258| 	if (this.change.Limit)
| 259| 259| 		if (newAmount > 0 && finalAmount > +this.change.Limit ||
| 260| 260| 		    newAmount < 0 && finalAmount < +this.change.Limit)
| 261|    |-			finalAmount = this.change.Limit
|    | 261|+			finalAmount = this.change.Limit;
| 262| 262| 
| 263| 263| 	this.SetAmount(finalAmount);
| 264| 264| };

binaries/data/mods/public/simulation/components/ResourceSupply.js
| 261| »   »   »   finalAmount·=·this.change.Limit
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1064/

elexis edited reviewers, added: Restricted Owners Package; removed: elexis.Feb 23 2019, 4:30 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/ResourceSupply.js
38

And why is it useful to implement something like your first example,
i.e. what is the use case for a MaxAmount that is greater than every Limit of a positive change,
i.e. what is the use case for displaying a bar that indicates that a resource can have up to 5000 res while it actually can't?
If that's useful to display something other than the ground truth upper limit, why is it not implemented for the lower limit as well?
I'm unconvinced by this field for the proposed feature implementation, but I'm not telling you what to do.

39
  • Amount is nonNegativeInteger, MaxAmount is nonNegativeDecimal
  • MaxAmount still redundant with Limit
48

Use case for specifying a Change without Changes?

49

-Optional

61

Should use the type the other schemas timer delay/intervals use, same for the other properties

71

(change -> Change in the description, name capitalization indicates to the reader that the sentence relates to the the branded version of change that we defined above, not to be confused with whatever connotation the reader thinks about when reading the colloquial change)

259

ApplyChange? (if going for "Change")

271
274

string?

325

The restarting-timer problem didn't really go away.

Stan updated this revision to Diff 7497.Feb 23 2019, 6:34 PM
Stan marked 7 inline comments as done.
  • Fix the above comments, save from the Delay thing. I can figure out a great way to do it.

Disabling delay for resets by adding a param to add timer means that it will spam if the interval is reset too often.
Setting a timeout would just mean that timers are not cancelled properly.

What might help is using a debouce function, but I'm not sure it's the right way to go.

var JD = {};

JD.debounce = function(func, wait, immediate) {
	var timeout;
	return function() {
		var context = this,
			args = arguments;
		var later = function() {
			timeout = null;
			if ( !immediate ) {
				func.apply(context, args);
			}
		};
		var callNow = immediate && !timeout;
		clearTimeout(timeout);
		timeout = setTimeout(later, wait || 200);
		if ( callNow ) { 
			func.apply(context, args);
		}
	};
};

JD.firstName = function() {
    console.log('John');
};

JD.lastName = function() {
    console.log('Dugan');
};

window.addEventListener('resize', JD.debounce(JD.lastName, 400));
window.addEventListener('resize', JD.debounce(JD.firstName, 400, true));
binaries/data/mods/public/simulation/components/ResourceSupply.js
48

Nope actually thanks.

61

It does.

71

Right.

259

Fair enough.

271

Thanks.

274

Ah yes you are right.

Stan marked 2 inline comments as done.Feb 23 2019, 6:35 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/ResourceSupply.js
38

It's not actually. You're right with the limit thing.

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
|  89|  89| 	this.infinite = !isFinite(+this.template.Amount);
|  90|  90| 
|  91|  91| 	if (this.template.Change && !this.infinite)
|  92|    |-	{
|    |  92|+	
|  93|  93| 		for (let changeKey in this.template.Change)
|  94|  94| 		{
|  95|  95| 			this.AddTimer(changeKey);
|  97|  97| 			if (this.template.Change[changeKey].Limit !== undefined)
|  98|  98| 				this.maxAmount = Math.Max(this.amount, +this.template.Change[changeKey].Limit);
|  99|  99| 		}
| 100|    |-	}
|    | 100|+	
| 101| 101| 
| 102| 102| 	// List of IDs for each player
| 103| 103| 	this.gatherers = [];
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1065/

nani added a subscriber: nani.Feb 23 2019, 6:40 PM

This debounce thingy looks very nice, could be used in many places in gui.

Used as reference https://davidwalsh.name/javascript-debounce-function ?

Conceptually:
OnInit: If Change active, SetTimeout(OnTimeout).
OnTimeout: ApplyChange; SetTimeout(ApplyValueModification(Interval)) ?
OnValueModification: Not

This means the interval is not changed immediately, but only after the current timeout ran out. (One could check how it's done in UnitAI and try to achieve consistency and bug minimization as mentioned.)

Stan marked an inline comment as done.Feb 23 2019, 6:59 PM

Oh yeah ! I'd didn't understand how set timeout could work as set interval but I didn't consider it could be recursive. I guess it would simplify the code. I'll try it when I have some time.

@nani Yeah I use a similar version at work but that's usually because you use frameworks that don't have a good management or that send too many messages you have no control on.

Delay could also be modified by auras. Perhaps some use cases for auras would prefer to have instantaneous updates of Changes. So would really be better to compare it with existing implementation of timer intervals that are affected by valuemodifications. Should probably also check whether it adds a lot of overhead to call SetTimeout repeatedly as opposed to one SetInterval.

In D1718#72341, @nani wrote:

This debounce thingy looks very nice, could be used in many places in gui.
Used as reference https://davidwalsh.name/javascript-debounce-function ?

(Please no piling of random stuff)

binaries/data/mods/public/simulation/components/ResourceSupply.js
7

removed

61

Delay decimal, interval integer; yet they use the same value range (milliseconds used by timers)

81

// TODO: Should not serialize this.maxAmount and this.infinite
(A refs # to that serialization minimization ticket sanderd17 wrote wouldn't be inappropriate.)

94

Only the increasing changes can increase the max amount.

can nuke outer braces.

if (this.template.Change[changeKey]) (either undefined (falsy) or object (truthy))

Stan marked an inline comment as done.Feb 23 2019, 7:52 PM

There is no overhead in calling set Timeout that set interval doesn't has. See my timer.js cleanup patch.

binaries/data/mods/public/simulation/components/ResourceSupply.js
81

See the cleanup patch for resource supply. If it's committed first no need to add todos.

nani added a comment.Feb 23 2019, 9:16 PM

There is a important difference between setTimeout and setInterval (at least in Node and web browsers in general)

setTimeout calls the function again in X time after the function ends
setInterval calls the function every X time repeatedly (meaning it can overlap with the previous calls if they didn't finish)

Stan marked an inline comment as done.EditedFeb 24 2019, 4:39 AM

Doesn't seem to be the case. Set timeout just calls set interval l with a 0 repeat time.

elexis added inline comments.Feb 24 2019, 11:49 AM
binaries/data/mods/public/simulation/components/ResourceSupply.js
94

Should have maxAmount set before the timer is added, to ensure that nothing that this function could imaginably call reads from maxAmount before its set

275

If limit is optional and this code is used, it can reach numbers smaller than 0 and arbitrary great numbers

Stan updated this revision to Diff 7499.Feb 24 2019, 4:31 PM
Stan marked 7 inline comments as done.
Stan added a comment.EditedFeb 24 2019, 4:35 PM
  • Remove the OnValueModification function as it's now useless.
  • Use SetTimeout instead of SetInterval and call it recursively.
  • Fix using members that don't exist (this.change)
  • Do not serialize maxAmount unless it's needed.
  • Only apply max limit on growing entities
  • Reset Value ton an integer as it can be negative
  • Fix example
  • Use a nonnegativeinteger for the delay instead of a non negativedecimal as it is done elsewhere where the value is multiplied by 1000.
  • Set MaxAmount before creating timers.
binaries/data/mods/public/simulation/components/ResourceSupply.js
275

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.Sat, Apr 27, 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.Sun, Apr 28, 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 ;)