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 marked an inline comment as done.Feb 18 2019, 9:32 AM

Is it okay if it's threshold for both ? Can be a threshold below and above right ?

No, threshold is by definition a minimum value that must be exceeded (see https://en.oxforddictionaries.com/definition/threshold 2); limit is more generic and can work both ways (lower limit, upper limit). Moreover, having two separate values would allow entities that only change if their current amount is between e.g. 20% and 80%.

Stan added a comment.Feb 18 2019, 10:04 AM

Okay I'll add both then and make this a monster patch with 6 features in one. I'll also need to rewrite every single resource supply in the game. Sounds great. Gonna take some time though. Good thing we have inheritance.

Okay I'll add both then and make this a monster patch with 6 features in one. I'll also need to rewrite every single resource supply in the game. Sounds great. Gonna take some time though. Good thing we have inheritance.

Take your time; it's better to do things properly rather than quickly.

I'll also need to rewrite every single resource supply in the game.

Most resource templates (gaia) are pretty straightforward, but the field (<Amount>Infinity</Amount>) probably needs a bit more attention.

Sounds great.

Sorry to be so troublesome.

Stan added a comment.Feb 18 2019, 10:21 AM

Not your fault. I just wanted that feature really bad for the players and I did not expect having to spend more than 100 hours on it :/

I got my hopes up thinking I could do it. I guess I'll keep working on cleanup patches and writing documentation. Maybe unit testing. Btw @elexis if you could review the resourceSupply cleanup patch that'd be nice.

Yes, it would be great to have this feature in game, I already have quite a few ideas how I want to use it in my mod.
I could help with the templates, but not with the code, unfortunately.

Okay I'll add both then and make this a monster patch with 6 features in one.

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?
Adding Min/Max to the "changes", using SetTimeout instead of SetDelay to avoid restarting the timer upon interval change
The only downside is that one could add a useless Minimum value to a change that increases the resource count.
But being able to change the limits per change seems definitely useful, so I'd propose to implement it there.
Another reason to move the limit to the change effects is that it seems, at least in the code state after this commit, wrong to be able to specify a Max value but no <change>.

Btw @elexis if you could review the resourceSupply cleanup patch that'd be nice.

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

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

193–194

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?

282
  • 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?
325

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

332

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.

193–194

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.Sat, Feb 23, 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)

266

ApplyChange? (if going for "Change")

278
281

string?

332

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

Stan updated this revision to Diff 7497.Sat, Feb 23, 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.

266

Fair enough.

278

Thanks.

281

Ah yes you are right.

Stan marked 2 inline comments as done.Sat, Feb 23, 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.Sat, Feb 23, 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.Sat, Feb 23, 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.Sat, Feb 23, 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.Sat, Feb 23, 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.EditedSun, Feb 24, 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.Sun, Feb 24, 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

282

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.Sun, Feb 24, 4:31 PM
Stan marked 7 inline comments as done.
Stan added a comment.EditedSun, Feb 24, 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
282

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.Sun, Feb 24, 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.Sun, Feb 24, 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.Mon, Feb 25, 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.Sun, Mar 10, 6:58 PM
Imarok added a subscriber: Imarok.

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

Stan added a comment.Mon, Mar 11, 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.Sat, Mar 16, 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