- User Since
- Dec 21 2016, 1:38 PM (122 w, 1 d)
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).
Wed, Apr 24
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.
Tue, Apr 23
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?
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.
Mon, Apr 22
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.
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?
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?
Sun, Apr 21
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.
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.
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".
Sat, Apr 20
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...