- User Since
- Dec 21 2016, 3:52 PM (172 w, 2 d)
Dunno if the "is ready!" substring should be bold too, I guess it's a bit arbitrary. One can be conservative unless one discovered explicit reason to change something (as was done in the class rewrite). I didn't compare all chat messages, you can decide, patch seems fine otherwise. I suppose I can risk an acceptance without having tested. Thank you again for writing the patch.
Fri, Apr 3
Fri, Mar 27
I am a bit worried about the procedure of how the resulting patch is determined.
Thu, Mar 26
Previous revision D1662.
Wed, Mar 25
((Side node Documenting the use case / intention / objectives of the commit allows readers (including future self) to not have to guess that and to become able to search for use cases and find the commits implementing them.
Thats typically done in a summary of a revision proposal - if that is skipped it would be good to mention that briefly in the commit message to yield those benefits.)
Thank you for picking up nani's report and writing a patch for it!
Mar 10 2020
I suppose the value is cumulative
(I didn't check whether the message moving makes sense, or test it in any way, or anything else)
IsVisiblyGarrisoned does loop for each entity checked, perhaps that can be optimized, I didn't check (code only handled for garrisoned entities, so its not as grave as it could be, but there also was some 10s freeze when a ship was destroyed some releases ago, and there could be cases where a player deletes/renames/ownershipchanges 100+ entities with 10+ entities garrisoned each). (Also didn't check whether sending visible entities in the messages instead of having the message handlers call into the function is more performant.)
One cannot visibly garrison a unit that cannot be garrisoned non visibly
The fallback is needed for things like walls
Why is the check needed for walls if thats the case?
Mar 9 2020
Is the fallback redundant and thus useless (same as always allowed if not specified)?
Else the classes specified in the List tag will be used.
That works too
Mar 8 2020
Mar 7 2020
(can be fixed when committing and/or ignored)
Feb 28 2020
Disable TLS option from rP21924 has not been rebased as well.
Feb 27 2020
why is the Schema moved?
I suppose that's out of scope, but I would recommend against get syntax when there is the choice.
If we call a function, it should be visible as such.
If we actually want to have a prototype property, then it should be expressed as such.
Storing literals in the prototype is a use case, for example UnitAI globals, or
BuildingAI.prototype.MAX_PREFERENCE_BONUS = 2;
And IMO it would be ugly to add a function call and making it impossible to assign a value to that property name.
(Its possible to have one value on a given property name on the prototype and another value on the same name on an instance of the prototype, so the prototype value can be used as a fallback.)
Especially if there were many literal values stored in the prototype, having a get function for each would accumulate function ugliness quickly. (If we want an assignment X = Y; then why make it a function. Just for the single class scope shouldn't be worth it if it simultaneosly has these other effects on code, possibly even performance due to the +1 function call)
Use JS_HasProperty instead of JS_HasOwnProperty.
SpiderMonkey 45 contains known security vulnerabilities.
CVE doesnt list spidermonkey, so I suppose that refers to a subset of https://www.cvedetails.com/vulnerability-list/vendor_id-452/product_id-22101/version_id-202242/)
20:03 < elexis> "SpecialVariant" what does special mean 20:03 < Imarok> different from the usual ones 20:04 < Imarok> fields are special, so they get a special animation variant 20:05 < Imarok> "special": just different from normal 20:06 < elexis> then define what is normal 20:06 < Stan`> Philosophy 101 20:06 < Imarok> normal is what you get when you don't add this component ^^ 20:07 < Imarok> I thought about naming it BuildingDependendVariant, but it could also be used for other entities 20:07 < Imarok> So I stuck with special 20:10 < Imarok> I'm also not 100% fond with the naming, but please use constructive criticism and no philosophical questions...
I object to that notion that this wasn't constructive criticism and that finding a clear definition and name would be a philosophical and not development question, because the reader and template user must not guess what normal and special means.
We can see 60 variants defined in lists.xml, why are those 59 ones normal and the field one special?
Why are gather_treasure and syntagma_front_walk normal and not special?
Perhaps with normal variant you mean "default variant"? But there is no actual default variant I see.
Regardless of the distinction that one might find, is the distinction necessary or might it look better if theyre stored in one place?
It looks like the name of that new component would just be <VariantNames> storing the names of variants that can be used by an entity performing an order with the target entity of that template.
Feb 26 2020
Feb 20 2020
The bogus template is only for testing, not to be committed?
Feb 15 2020
Feb 12 2020
See http://irclogs.wildfiregames.com/2020-02/2020-02-12-QuakeNet-%230ad-dev.log; textual diff is identical, binary diff has as a first change one more entity in the JS Map stored in AI entitiesModifications.
Feb 10 2020
- Computing minColumns / maxColumns (zoom limits) depending on image aspect ratio and current available vertical and horizontal screenspace and minimum image width only
- Disabling the Zoom buttons when the zoom limit was reached
- Disabling/enabling the Select button if there is no selection (selection removal can happen if there are no text filter matches)
- Deleted the png, too saturated colors, used a "Browse Maps" Modern themed button below the MapSelection dropdown and assigned the mappreview image click to open the mapbrowser too
- Added the generic GameSettingControlButton class to achieve that.
There is "CNetMessage: Corrupt packet (incorrect size)" already.
Feb 9 2020
One abs less.
More static cast, drop old tests, use macro more often, rename macro.
Ditch Fixed as an arugment and implement FIXED_SQUARE_I32 macro
- TODO: If currently selected map is not part of this list, remove the selected index
- The missing scrolling is really bad. I didnt check your JS scrolling implementation, but a slider might also work as a workaround.
- The code doesnt work as intended for clients (that aren't the host) unless I missed something reading the code
Feb 8 2020
The size property of GUI objects can be changed to the value return by GetTextWidth, there is an example in termsdialog.js, or search for Engine.GetTextWidth. (I suppose it'd be better as a C++ snapping or alignment feature.)
- Item-Description: Should(n't) the map description be shown (maybe 3-5 lines + scrollbar) below the mappreview thumbnail, so that the player can make a more informed decision / comparison of maps? In fact the height of the description could vary, so that the unused vertical space would disappear as well (at the cost of perhaps one or two rows depending on zoom level)
- Status-Bar: Should(n't) it display the "prev/next" + page counter data below the items in a status bar? (It could also display item count)
- The mapbrwserpage button looks weird, we considered making this a regular button.
- GridBrowser class (D1650) split from MapGridBrowser class to keep it more reusable.
- GridBrowserItem and MapGridBrowserItem class, so that one may keep references, a local state, register events to handlers and specify functions per item (instead of only one function doing all of that locally).
Feb 7 2020
See #4252. Engine.GetTextWidth would yield the actual size used, avoiding the issue that a fixed number of pixels may look ugly for english while still being too few for other languages.
Feb 5 2020
(For reference, there is also this bug #5675, where the simulation sends a wrong victory chat notification when ceasefire ended after the only player of the game had won already, showing " and undefined have won". But I guess that victory message should just not be sent, then it probably won't affect this patch.)
Squaring is no fun, because it uses Multiply and Multiply overflows without printing a warning (while warning when overflowing when casting).
Need to use the internal value (u64) for comparison, because fixed can only represent 2*128² at most and one often receives vector lengths an order of magnitude or more greater than 128 that need to be squared.
Fix rangemanager missing parentheses, add testcases proposed by Vladislav
Feb 4 2020
I agree with the proposed string changes being a more clear description, but I wonder where we transifex is in translation coverage currently (whether they keep up with translations as fast as we introduce or change strings).
Another batch of code comments, not finished yet. Mostly the code should be restructured to use event-subscription to decouple the different classes.
(To be fixed in ./binaries/data/mods/public/maps/scenario.rng ./binaries/data/mods/public/maps/scenario.rnc )
Feb 3 2020
I wonder why champions are significantly faster than citizens. They have a lot more armour and health and can't gather (hunt). If anything, I'd expect champions to be slower or at most equally fast.
I guess the argument would be that they are more trained and elite, but indeed that needs to account for the weight of the additional armor.
The objective of the diff is to avoid repeating this operation in loops where this value doesn't change during the loop:
i32 c = cmp.GetInternalValue(); u64 c2 = (u64)FIXED_MUL_I64_I32_I32(c, c);
- Add Vladislavs test case
- Drop fractional part here as well.
The test came from http://irclogs.wildfiregames.com/2019-07/2019-07-12-QuakeNet-%230ad-dev.log
19:02 < Vladislav> fixed n = fixed::FromFloat(1.0f / 10.0f); fixed n2 = n.Square(); CFixedVector2D vec(n, n); fixed len = n2.Multiply(fixed::FromInt(2));
19:02 < Vladislav> elexis: what should we get for vec.CompareLengthSquared(len)?
19:07 < elexis> according to vec definition, squared length of vec is 2*n², that is the same as len, therefore vec.CompareLengthSquared(len) ought to be 0 as the numbers are the same
Feb 2 2020
(Following discussion on http://irclogs.wildfiregames.com/2020-02/2020-02-02-QuakeNet-%230ad-dev.log and before (on days when a new patch was uploaded))
I'd rather buff them again in a24 if they turn out to still be too weak.
I agree with that.
Feb 1 2020
Though it can also be explained as them carrying having carried those resources.
Carried resources are looted separately, so that doesn't really apply. Doesn't seem like a big defect since there are rarely more than 3 elephants around.
Jan 31 2020
Jan 30 2020
- data.player || player expression from rP17617
- !!data.owner ? data.owner : player from rP23237 (called in https://code.wildfiregames.com/rP23237#inline-4598)
If I read correctly, this.sentDiplomacyRequestLapseTime is randomized, then replaced by the deserialized value. So the code change is not necessary but done for consistency, because the random calls are considered the root cause of the problem?
If all of that is the case, it seems that the more safe fix would not be to remove the randomization calls, but to serialize everything that needs to be serialized.
Jan 29 2020
From testing the patch:
(FWIW got an absence of objections from nani http://irclogs.wildfiregames.com/2020-01/2020-01-29-QuakeNet-%230ad-dev.log)
Reasons provided for the cost and effect changes are reproducible, have been tested, are necessary but not excessive, therefore a correct variant.
Didn't find the string loom in json, js and xml files other than this, so changes seem complete.
Missing test/ files -.-
Missing test setup file
options.json diff and default.cfg diff can be considered accepted by me. The JS diff looks ok, but I didn't check for completeness. Duplication removal and code minimization is good.
60px*60px = gateway to a23b rerelease hell, we need potency of 2 imagesizes, the NPOT ones crashed on macOS only in the last release.
There are two different ways this could be implemented.
Jan 28 2020
forgotten about that I had to explain where to find it and what it does to a bunch of people