HomeWildfire Games

Merge resource agnostic branch by s0600204, fixes #3934.

Description

Merge resource agnostic branch by s0600204, fixes #3934.

Remove all occurances of hardcoded resources in the simulation, GUI and AI code by
specifying resources as JSON files in a new simulation subdirectory and
accessing them through a globally defined prototype.

Details

Auditors
s0600204
Committed
elexisNov 19 2016, 3:29 PM
Parents
rP18963: Support hardcoded translation comments when extracting strings from JSON files.
Branches
Unknown
Tags
Unknown

Event Timeline

Someone shoudld probably raise a concern

Also D23

/ps/trunk/binaries/data/mods/public/simulation/components/Cost.js
26

As reported by fatherbushido, this now uses nonNegativeDecimal instead of nonNegativeInteger for reources, while before only the time had that type.

This commit now requires audit.May 5 2017, 2:45 AM
s0600204 raised a concern with this commit.May 5 2017, 5:23 AM
s0600204 added subscribers: fatherbushido, s0600204.
s0600204 added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/Cost.js
26

Tracking back through the history of #3934, versions of the then-proposed patch up to and including v6.1 had nonNegativeInteger, but in v7 this was changed to nonNegativeDecimal.

The change was made to ensure compatibility with rP18500, which was committed by @fatherbushido in an attempt to resolve #3818. That changeset was later reverted in rP18632, but it appears that subsequent patches to #3934 (or, more accurately, commits in either of our respective git branches) did not take this into account.

As far as I'm concerned, if there's no need for cost values to be decimals, this line can be changed to use nonNegativeInteger. However, there's not much point in changing it if the ultimate solution to #3818 does require it to be decimal.

This commit now has outstanding concerns.May 5 2017, 5:23 AM
fatherbushido added a comment.EditedMay 5 2017, 10:51 AM

Nice unknot!
Yes it's better to close that tangent universe ;-)

I've given this a few thoughts and I think we will need a separate system for the "rounding" that needs to happen, because the validation should not have an effect on the templates imo. So something like adding

<SomeItem op="mul" round="true">2.5</SomeItem>

to the templates when inheriting. Which means that this can be changed back to NonNegativeInteger ultimately.

I've given this a few thoughts and I think we will need a separate system for the "rounding" that needs to happen, because the validation should not have an effect on the templates imo. So something like adding

<SomeItem op="mul" round="true">2.5</SomeItem>

to the templates when inheriting. Which means that this can be changed back to NonNegativeInteger ultimately.

Ah yes that would solve the problem of not have the validation schema when constructing nodes. I had thought first to use imul or iadd operation but that sounded ugly.

s0600204 resigned from this commit.May 17 2017, 8:14 PM

I probably shouldn't be the auditor of my own changes...

So, are we all agreed that nonNegativeDecimal should be changed to nonNegativeInteger in simulation/components/Cost.js line 26? If we are, I'll commit that one specific change, and we can count the discussion here thus far as the "Review" of that. (Unless someone really wants a Revision creating.)

This commit no longer requires audit.May 17 2017, 8:14 PM
fatherbushido added a comment.EditedMay 17 2017, 9:41 PM

I probably shouldn't be the auditor of my own changes...
So, are we all agreed that nonNegativeDecimal should be changed to nonNegativeInteger in simulation/components/Cost.js line 26? If we are, I'll commit that one specific change, and we can count the discussion here thus far as the "Review" of that. (Unless someone really wants a Revision creating.)

Yes (according to rP18632).
It seems that nothing change meanwhile using decimal costs so it should be safe.
I don't know if you need to create a diff, perhaps it's better, at least someone can launch a validation script before (or the test units map).

elexis added inline comments.Feb 25 2018, 7:29 PM
/ps/trunk/binaries/data/mods/public/gui/session/session.js
512

This seems problematic, we want only one way to access resource tooltips.
Should we use resourceIcon(?

s0600204 added inline comments.Feb 27 2018, 9:43 PM
/ps/trunk/binaries/data/mods/public/gui/session/session.js
512

resourceIcon() returns something that can be included in textual strings. You might note that this line is not setting a textual string, but a sprite.

The images used here and the ones referred to via resourceIcon() aren't even the same files ({res}.png / {res}_small.png).

And the tooltip for these top panel resource icons is not got or set here, but in updatePlayerDisplay().

So... what's the reason for wanting to use resourceIcon() here?

elexis added inline comments.Feb 27 2018, 10:28 PM
/ps/trunk/binaries/data/mods/public/gui/session/session.js
512

Just noticed while reading a related diff while reading a related diff that we don't have a single source of truth policy here. Would be easier to manage for mods if they have to specify the place only once. Might not be possible to achieve however.

bb added a subscriber: bb.Jun 4 2019, 11:04 PM

While duplicating a file in D1936:

/ps/trunk/binaries/data/mods/public/globalscripts/Resources.js
47

So we are adding them here 1 by 1, to later sort them anyhow, looks like we should rather use a .map on that other sorted array below...

54–56

(a, b) => a.order - b.order; looks like doing the same

93

while at it a .