In D1828#76156, @elexis wrote:

Seems unlikely that GetHitpoints returns something other than this.hitpoints, especially inside the same component.
I know the JIT can optimize, but I don't know if it can achieve this specific thing.
It's easy to change this.hitpoints if we ever need some logic change.
If we change it to use the function, then we should have a reason to (i.e. construct a realistic example where the logic would be different from return this.hitpoints).

In D1828#76107, @elexis wrote:

But why replace this.hitpoints with this.GetHitpoints()? Only adds performance cost without adding benefits, doesn't it?

Will update with your comments, except for the "runspeedmultiplier" thingy.

I think perf impact is negligible (it's definitely better, but probably not by much, and the main impact is likely a lower number of draw calls).
Probably a good idea to do it anyways though.

It is indeed... We don't use the jpc at the moment so this doesn't crash, but it should be fixed.

Yup, that ENSURE was broken... Mind you my initial code was _also_ broken since at() would have failed here too. Apparently I didn't playtest that revision and we have no tests calling ComputeJPSPath, which we really ought to (might add some this WE).

Foam really isn't that useful anymore either, so I guess commit anyways.

I think I'll check out how that changes because that might have been WAD by me tbh. Up to personal preference I guess.

Thanks @Itms , @Stan and @vladislavbelov

In D1804#75891, @vladislavbelov wrote:

Mind you, foaminterp.x == foaminterp.z. So it equals to:

foam1.x = foaminterp.x * (WindCosSin.x - WindCosSin.y);

I think I also changed build/workspaces/update-workspaces.sh

So you acknowledge that the commit may actually be wrong and for instance obfuscate instead of resolve the issue, or fix the issue in one case but still come with a configuration that we don't want, or possibly even breaking under some circumstances, but you are just not concerned about that?

In D1772#75868, @Itms wrote:

The issue might be in our code (and it might not) but I don't have time to debug it properly and/or won't do it anyways as I already have plenty of things I care to do on this project.

I agree with Itms, if you don't have the time or will to do something properly, then don't do it.

Yeah I'm not sure why you went ahead. I'm happy it's in, but you could have left it aside if you wanted, and it would have been thoroughly reviewed in the same way as the other macOS TLS patches from the re-release (probably at a moment where it would become high priority, like the A24 release). Did you feel pressured to review it somehow (this is what your message looks like)? Or did it look like a low hanging fruit? I'm interested in knowing that in order to make sure the priority order of reviews in done in a sensible way.

Well whoever gets to test it first commits the fix I'd say 👍

Any chance you can test that?

Annoying... Can anyone raise a concern? I'm not sure I'll have time to fix it today/tonight - also don't have VS13. I'll have time tomorrow if needed.

I'm OK with the reasoning. Some comments.

Rm, on second thought, I think we need to keep the original headers in most files. For one thing this would reduce the number of changes (which helps with rebasing old patches and is generally nicer) and for another thing I need to look at what you're actually putting in the precompiled headers. Some of them might increase build times from scratch but cause a bit of pain when modifying these files...

Should run on top of rP22210.

Reupload after dependent commits have been merged.

Mh, didn't think of running that script indeed. I'll fix it, thanks.

...Because of course I did...

This is next on my commit list (once builds have passed for the rest) as it is needed for D1832 to cleanly apply, and I'll push new tests proving D1832 then, then commit that, which enables running D1834 on top of all these tests, which I then feel reasonably confident to commit soon (I'd like to run an MP replay with it to ensure hashes are consistent though).

There was... Thanks for noticing.

In rP22206#32931, @Stan wrote:

You fixed disabled tests but did not enable them ? :)

I would suggest that C++ GUI lists are always "multi select", but we can enable/disable the actual multi-selection part. So change 'selected' to be a CIntList, which would return a one-sized array. Then JS code wouldn't have to awkwardly handle two cases.
That would imply modifying a lot of existing JS GUI code though. But it sounds greppable. So I'd say give it a shot, if it's too difficult I'll change my mind.

TBH this is too complex and too complicated for my tastes as it's written. I say 'remove stuff'.

Following elexis' comments (I'll link this one but see the whole thread), adjust variable names for clarity:

@Itms Hm, so what's the status on this?

In D1752#70523, @elexis wrote:

There will certainly be people with only 2 buttons (Wasn't that about every mac user?)

Nowadays mac users have no buttons at all and touch input, in fact ;)

Might also take a look at this.

This does sound like a 'core' c++ feature to me, but having a JS implementation first and then possibly porting it to C++ sounds like an interesting approach to me.

Care to do the siege units as well?

Open question for other devs: don't we already have a patch for trample damage somewhere?

I'm wondering the real usefulness of the bolt shooter vs stonethrower templates also. It seems to me basically all "shooter" siege engines of the time were torsion engines, which could fire smaller or bigger projectiles and arrows or stone sort of indifferently.
The main difference was mostly whether they could shoot precisely (aka be used as a marksman's tool) and mostly horizontally to target enemy units, which was in general a factor of size it seems.
Might be more interesting to have a "light" and "heavy" variant or something. Or just merge them and specialize derived templates since siege engines were kind of civ-specific beyond rams.

In D1840#75562, @elexis wrote:

If the value that SetSpeedRatio computes differs from the serialized value, by definition it's OOS?

In D438#75552, @elexis wrote:

(See also comments in rP22197.)

But m_SpeedRatio is serialized and then replaced by m_SpeedRatio = std::min(m_SpeedRatio, m_RunSpeedMultiplier);.
This sounds like it would go OOS if that line changes the value,
or if the line never changes the value, is a useless line (misleading the reader to believe that it has use)?

See D1840, but this is only to check if the "new" m_RunSpeedMultiplier isn't actually lower than m_SpeedRatio, which can happen after an aura/tech/ownership change.
Deserialization actually uses this call solely to recompute m_Speed.

Mhpf, you're right. Fix other whitespace in this file while I'm at it.

See D1840.

This code is safe.

Remove the helpers mock for now.

Hopefully clarify comments.

About creating new files, Im skeptical if they are 120 lines short, since the other files are couple of thousand lines long

@elexis I'm inviting you to look at this, from playing around with it it feels much nicer to me.

More complete example of what I'm envisioning. We'd have possibly multiple "Input State" components that listen to events and possibly 'kill' themselves - so no transition from an input state to another input state except "no input".

I feel extremely "meh" about this :p
Commit it if you want, don't if you don't. It's unlikely I'll do it myself is my point.

Wouldn't it be better to max(0, ...) the whole calculation? This is magic-graphical-code but you're changing behaviour a lot here.

Remove the "only if blocking pathfinding" because I don't see the point tbh.

At this point it'd probably be usefully rebased on top of D1834... I'll keep this in my review queue as I might commandeer at some point in the future.

I'm not sure how useful this is. We're linking statically anyways and this is OSX only so...