- User Since
- Dec 21 2016, 3:52 PM (121 w, 2 d)
make the run multiplier optional and handle that specifically
That's what I was wondering about - whether there was a limit check in place before somewhere, I didn't find one from a quick look.
(I guess the previous code was not incorrect, but adding the const adds benefits)
Formations have a 100 run multiplier which effectively sets their maximal walking speed at 100
Indeed, why is that run multiplier 100 and not left undefined, or to be defined by the specific formations?
Very informative description by Angen that was used in the commit message, thanks for writing and chosing that.
(I hope you have examined possible edge cases)
Sun, Apr 14
@vladislavbelov should this code not be in other source/gui/ code where all the other GUI code is, for two reasons (1) cohesion and (2) performance (onTick)?
About (1), I suspect C++ has access to things that JS doesn't have access, but would either required or useful to be accessible / interactable / configurable in this context.
But I can't make a conclusion without having seen the thing in action.
Thu, Apr 11
Wed, Apr 10
I didn't test, but I guess it works as expected now. (I didn't figure out whether the bugfix is the cleanest way, I suppose I can still do another concern if it was that bad.)
Sun, Apr 7
That it attacks and captures simultaneously is a bit in contradiction with the rest of the units that can either capture or attack at a time. I see this making the unit probably OP if not nerfed somehow otherwise.
I wonder whether it makes sense for a siege engine to capture - I thought the men are rattling at the door.
Due to popular demand, UnitAI.js should support capturing for units with buildingAI anyhow.
Sat, Apr 6
15:34 < bb> just leaving it accepted for a short while (till tonight) in case someone wants to add something
21:20 < nani0> btw vladislavbelov[ how do you organize all your current projects (pending patches), do you use git branches or just save *.diffs?
(Branches are useful if you have multiple patches that depend on each other, so you can rearrage the commits and pop one of them at a time into the review queue. If it's a standalone patch that doesn't have merge conflicts with others, you can create a revision proposal or paste and then there is one backup of it. Also I used a messy directory per year/month/day for 0ad notes, abandoned patches etc.)
Somewhere between 30% (most cases) to 50% faster.
Thanks for testing it, sounds promising. I will try too once I open my IDE again and closed some hundred revision proposals...
Fri, Apr 5
I can't read the last two comments now, so I might repeat things mentioned by bb or say something that was responded to by nani. But I add some comments sooner rather than later:
(I meant we can find the answer in the specs if we are not sure)
What does the C++ specs say what happens?
The tooltip lies about elephants, quite a stuffed tech, but it should still work out, the part +15% melee and ranged attack damage could be +%(AttackMeleeHack)s%% melee and ranged attack damage, still seems less than more likely to mess up the numbers, as one now only needs to compare that ranged and melee attack is the same, instead of testing that ranged and melee attack and the number in the tooltip are the same?
Don't forget to add the references in any text field (rP21113), this way the referenced site will also receive a link to this document and readers can click their way through history.
Would improve generated maps times by 3~4x times for all sizes if combined with the other diff of TileClass.
The factor 3-4x is still misleading nani (see post above).
If the Boolwrapper would make it 50% slower than the current implementation, then the other patches combined could be 6-8x as fast.
x + y + z = 3 doesn't tell us that x > 0, it could even be x = -100, y = 101, z = 2.
Thu, Apr 4
That .8 was necessary because its a really powerful effect (healing 200 units nearby simultaneously) and it degenerated the game balance towards people always having to train the unit.
The question is whether that one unit having one more digit than all the others makes it right to change all the others.
What if an upcoming template change uses 0.75?
If the objective of this diff to achieve consistency, one can also use consistently avoid trailing zeroes.
Wed, Apr 3
What's the benefit to having .0? Didn't it use round numbers everywhere?
I guess every improvement can be defined as a defect. Files should be grouped by commonalities, right? The less fragmented it is, the better. I didn't claim this to be a good idea, was just exploring thoughts. I'm sure the settings/data/ folder was once grouped by the fact that it's JSON.
I don't think that the header would be more clear, only smaller.
(I meant the includes, not the header. Sometimes we find something like 30 includes in a file with 3 classes, 10 of them for one class, 10 for another, 10 for the last class, then it's hard to tell what is forgotton, what is unused and what include is used by which class (here are no includes, so there's that))
Then add it to CC.
CC is to allow people to chose between multiple syntactically and semantically correct solutions.
It's not a convention that we avoid bugs.
It could be added under a different chapter or different document: Common Pitfalls or so.
Hm. I think auras and techs are also templates, just not entity templates.
special/ does not inherit from any template_*.xml
Currently all settings besides one or two IIRC take effect immediately, so players expect these to take effect immediately too.
Therefore you either have to mention that it requires a restart, like we do with the other settings, or actually make them take effect.
So it's good that we publish the version history of our source, so people can find this information.
But even better might be adding the comment to the code itself, so that people stubmle upon it while reading the code.
doesn't belong in other/ (which contains various gaia structures).
Confirmed, looked at all the other files in there.
Makes me wonder why that folder is called "other" and notthing relating to "gaia"?
So should the "other" folder be moved to gaia afterwards?
Tue, Apr 2
Mon, Apr 1
You did a good write up on the logic change, not the syntax candy change at https://wildfiregames.com/forum/index.php?/topic/25594-c-tip-1-constant-references/&tab=comments#comment-371946
Should mention it here too, at least that there is such a logic change at all.
Thanks for the requested response (I only recalled you spoke about doing something with C++, but not which aspect would be moved.)
Sun, Mar 31
On the importance of the feature:
we need to know how much memory and performance cost this influences.
The default allows to ignore all the events that one might not want to process but still happen (e.g. if in a state "PLACING" you only want to process "onmousemotion" event and ignore all the other types of events) otherwise the FSM will warn you every time you try to process an event in that state that is not defined. Mostly the default method should be an empty function in this FSM case
All in all, these possible unknown "default" calls that might happen can be easily seen uncommenting a warning in the FSM.js file to see what is being called at each processed event.
Sat, Mar 30
Fri, Mar 29
The fsm.js has been modified to be able to have a default function and "_" in state name
What uses the underscore?
Defaults are an anti-pattern, because
(1) it means the caller omits mentioning the value, so when someone searches for the value, he won't find all matches
(2) the behavior of the default value might go unnoticed by the caller (assuming that default always fits because that's why it's the default) but mentioning the value makes the reader more critical as to whether the value is correct, and informs the reader directly which value is used.
(3) is all calls explicitly state the value, then it allows comparison between the calls, and in some cases one might find a better value than the default
That argument is mostly for function argument defaults function x(a, b = 123), I didn't investigate what you did with this default here, but I expect it will match the same pattern.
Thu, Mar 28
Performance is of lesser importance; range overlays are options that can be turned off.
Don't underestimate possible consequences of bad performance.
An simple aura bugfix in a23 made the code so slow that it consumed 10 seconds (i.e. 0 FPS, full freeze) when starting a game, that in turn disconnected most players of a match, aborting the game after people clicked on start, basically every time. That was terrible for the players.
You don't want freezes under any circumstance, and a freeze is nothing but CPU taking that long to compute the current operations.
Wed, Mar 27
Thanks for the measuring.
Tue, Mar 26
If you want to commit this patch to continue working on greater features that depend on this, and if you have done your best to ensure the correctness, I would suggest to take that decision to commit into your hands.
Mon, Mar 25
21:45 < nani> does someone knows/has done FSM (finite state machines) before?
You can find authors in the revision history "svn log", or in the webbrowser on Phabricator -> Diffusion or trac -> browse source for FSM developers.
But I guess there are many UnitAI devs and only one or two absent devs who messed with the FSM code. I guess you need more a GUI reviewer who is familiar with input.js than FSM, since FSM can be learnt more easily than gaining a complete picture of input.js.
Well, I meant that one should try to work with the definition of the component properties more than with what the implementation currently does.
Who says that UnitAI is the only component to initiate a position change? It could also be a trigger script relocating a non-unitAI entity.
Or if the terrain is changed, then the height, and thus the elevation bonus can change as well for an entity.
There could be new components for arbitrary reasons that trigger position changes, it could be some scripted AI, or someones experimental alternative unitAI, or a magic tree that is moved in a cutscene.
Well it says "Display correct ranged overlay for maximum range".
If one wants to intentionally semi-support it, then one should mention it in a code comment, otherwise people think it's just something missing that can be added, rather than an intentional omission.
Sun, Mar 24
Sat, Mar 23
With the new message subscription, one could patrol on a hill with a unit that has the range displayed. Then the thing would be updated very often.
Didn't we have a ticket for it? I recall receiving a report about that once.
I also seem to recall having looked into the code and having gained the impression that range bonus was already accounted for.
Actually I get it again looking into the code and wondering why the elevation code in GetRange doesn't already provide the information.
I guess ElevationBonus != GetElevationAdaptedRange, the former must just have been height of the building.
Mar 21 2019
Actually, would have been a good opportunity to move the strings from public-gui-other.pot to a new translation resource, so that strings like 2000 Food. are not in the same pot as the majority of the GUI. Too bad I didn't think of that sooner. (Though I'm not sure whether the transifex webinterface offers previous similar translations of different resources.)
Mar 20 2019
Now the question remains how we make it visible to the players that this click-feature exists.
Usually the mouse cursor changes when one hovers a link. Underlining won't be supported by our small font base, but adding a sprite should be easily possible (resource icons in tooltips.js / setup_resources.xml as an example).
Mar 19 2019
(See rP22134#32587 on the JSInterface question.)
Unless the object doesn't contain text, in which case the method will return undefined`
I thought you mean empty string (that should return 0 size instead of undefined, because the size is known for empty string),
but you mean absence of a caption property from what I gather.
Mar 18 2019
I'm pointing out these things mostly because I want to inform on our coding conventions and good practices.
But I think the entire file needs to be revisioned structually, adding some syntax sugar is not the essence but part of what needs to be done IMO.
And the more stuff is added to the patch, the harder it will be to review.
Fixing 6000 lines in many logical and syntactical ways is something that can be done in a github branch and typically consumes several weeks to discover all the defects and the ideal structure. Then one can refactor that into small disjoint, atomic, patches.
(I guess the code isn't really inviting to newcomers who want to add one feature and are confronted with a piling disarrangement.)
Mar 16 2019
You incline me to agree with the idea of the patch, as it's selfcontained and only changes one little thing.
Would use arrow-functions while at it.
The question is how it should be ultimately structured and whether this is a step towards that.
Theres a ticket for refactoring this file, should be referenced both ways.
If I recall, this file is 6000 lines long, so I guess there's a lot more stuff to clean up there. A rabbithole for 2-4 weeks I guess, meh.
I'm not sure if some of the humongous object shouldn't be split into smaller objects.
For #5387 I had the idea that every file would be 200-500 lines of length, so that newcomers don't have to learn 6000 lines of relevant context to understand one file. I didn't plan how to structure this file however.
smiley claimed to have deleted his password, so I guess he can't remove himself from the subscription list now.
Thanks for the documentation fix!
Here janwas paper "Optimizing File Accesses via Ordering and Caching":
Mar 15 2019
translate(sprintf -> sprintf(translate
Mar 14 2019
(The hangup was implementing the JS diff?)
(As mentioned, I don't want to lobby for one or the other implementation, more than I want the decision to become as far-sighted and well argued as achievable)
Mar 13 2019
In this patch, IsInfinite() is called only in TakeResources, i.e. comparatively rarely, the string to number conversion is cheap and the isinfinity test is cheap too.
A JS Number can store what a C++ double can store, i.e. up to 2^53, so at least 64bit for every resourcesupply entity. That'd be at least 8 byte * 10k trees = 80KB. Probably 250KB with all the memory management overhead. Quite considerable if we only need it very rarely.
Actually there could be 150 gatherers * 8 players = 1200 units calling TakeResources every second.
But the cmpFogging.Activate(); will have a much worse performance effect than the isInfinite function call.
The patch was meant as a preparation for D1718, that also only uses it once upon Init.
Ok, that could work, but what if someone wants to use two lines as heading out of some reason?
I think we shouldn't overcomplicate this issue...
Then let's don't.
Is there a case to use two lines as a heading, or two lines to the same bullet, and if there is, why doesn't it use one long line and relies on the textwrapping?
If someone comes up with some more desires for this format, they can use JSON, or straight XML.
If one wants to argue with freedom to edit, XML would probably be better in the long term, since it could also specify the image to be shown and other arbitrary sprites.
So yes I think (and it seems like you do too) it is best to modify each line.
The difference is that in the one case every line has to be changed and every translation of every line has to be redone everytime one changes the style, as opposed to every line having to be touched only one and then never again if one wants to change the style (but only the one line style definition).
Doesn't sound like a good alternative, as some language might want to e.g. add the symbol at the end of the line...
How could we decide the memory/performance tradeoff?
Is it really better to modify every line to use that `•? Translators will have to edit it again. They might do so once, but what if we like to chose a different styling, for example an actual texture?
Alternatives would be keeping the "-" and replacing it if the line begins with that character (sounds like keeping ballast);
or deleting the - and prepending an arbitrary symbol to every line but the first.
XML would be an alternative but had the disadvantage that one can't use the same GUI object structure for every tooltip.
JSON would be an alternative, but I don't see where the txt format is worse than JSON for the loading tips.
Mar 11 2019
So the question is whether IsInfinite is called so often that it's worth the cache variable, considering that there can be 10k resource supply entities (trees), or whether the additional memory usage is worse; same for this.cachedType.
Mar 7 2019
The only alternative implementation that I can imagine is to keep the cache property, thus avoid the repeated parsing + infinity test, while still not serializing the property.
That means there is a higher memory footprint (not unlikely tens of thousands of trees) * few bytes but also many less CPU cycles.