In D438#63576, @bb wrote:

For some reason (couldn't quickly find why) a formation moves extremely slow right after it is created, when reforming (as in setting another shape) the problem is solved.

It's used to pass data to the AI, so no AIs -> useless. Order shouldn't matter here unless there's also an OOS bug in the AI Interface.

Looks good, clean and easy optimisation. Should be committed for A23 rerelease imo.

I fully intend to commit this/let itms commit this before the SM60 thing. The upgrade to SM45 is fairly safe, the one to SM60 really isn't and will need much bigger reviews.

If an A23 hot fix, commit as is and raise concerns. If not, don't commit and handle the C++ changes.

Sure.

OOS were easier to debug when they were really "bad" :p
Cheers for this!

In D1519#62415, @elexis wrote:

In D1519#62413, @Stan wrote:

In D1519#62412, @wraitii wrote:

m_Unit is basically a part of graphics, so shouldn't ever be used to stuff that affects the simulation and/or may be serialised.

Do I have to change something for this ?

Only if you changed a different place where a serialized value depends on m_Unit

m_Unit is basically a part of graphics, so shouldn't ever be used to stuff that affects the simulation and/or may be serialised.

I tested' yesterday's version and it worked and was very similar.

I don't know if there is a bug in the VFS. If there is, it should be fixed.

Well zip files are supposed to be faster than non-zip-files, so this sounds like a bug to me. I'll make a patch as said before

But no matter how good the code is, it has to open multiple files, possibly large fragmented zip files, reading mod.json from it, parsing the JSON data, possibly dozens of mods, so I doubt that this can't be optimized to microseconds without caching it.

Agreed, I was just messing around.

Agreed that after much debate this is the best solution.
I'll compile out of doing-the-right-thingness for accepting but this is OK by me.

This should fix the build and the JSPropertyDescriptor thing, which is fixed upstream https://github.com/mozilla/gecko-dev/blob/master/js/src/jsapi.h#L2049

It's more of a "it's good practice to not call constant things in function bodies", but I agree that it ever so slightly lowers readability here.

IMO OK once requested changes are done.

I think the lobby change is still OK, but I'm fine with not adding it for slight readability reasons.

The mountMods prerequisite is a little awkward indeed.

As a c++ caching method, this is probably the best, but obviously the question is "when to cache the data". I'm okay with once in Init like you've done, otherwise it should probably go in GetEngineInfo with an if of some kind.

Let's do it your way then, I'm not dying on this hill. Feel free to commandeer with your patch, otherwise I'll take it as a basis and carry on myself.

In D1512#61930, @elexis wrote:

In D1512#61928, @Imarok wrote:

Could we just do a small and quick fix (common js only cache) for A23 and think about the approptiate fix for A24 after the rerelease?

Committing something without knowing what investigating the situation prior to the commit is not happening with my agreement and the matter is not hard to understand.

@elexis : I don't really understand why you treat JSON as only the "way to natively store JS value", I see it more as a random document storing method, like XML. But whatever. Maybe this isn't the best approach but it seemed clean enough.
I believe initialising spider monkey could be done independently from VFS which is a very 0 A.D. thing so that sounds safe, but we'll have to see.

VFS changes are un-necessary with the C++ caching. I still think they are correct, but probably for another diff.

Alternative patch with C++ caching.

So I misunderstood that I couldn't reproduce this, because I could.

So the real cause of this freeze is definitely that we're fetching the data for every game instead. Then this is also perhaps slow on Windows for some odd pathological reason in the bundle that we should investigate (possibly windows is dumber about reading big zips than we thought).

FYI My plan is to commit if it doesn't appear to brea kantyhing and immediately raise a concern.

This is slightly ahead of @Itms' branch, and was created entirely independently, and we have mostly the same fixes (I just dropped scope chains, personally, since we used the global object).

Haven't read older code but this now looks quite sane. The rule with UTF-8 (which we sadly haven't always followed) is to always use it and never handle it in any particular way unless you're rendering text.
We should probably validate that keyboard input works correctly.

Thanks for trying this out, really helpful.
I haven't run into this linker issue, maybe because I haven't tried compiling the debug spidermonkey? Anyhow it looks like it'd be fairly easy to write a tailor-made patch.

Code looks good now, I just have a bunch of remarks. I'll try and test this soon enough and actually accept it.

Works correctly for me.

Transparent image.

Low hanging fruits: const-correct, remove unused functions, reorder things.

This is RC review-wise. I've tested it a number of times and it returns very similar paths to SVN, as said above it's faster, and I have a number of other revisions that conflict with this so the sooner the better.

These are irrelevant since visualActor overrides this. Likewise in D13. I think I'm removing them there already, so depending I might commit this or close it.

Go ahead and commit this in A24, just remove the garrisonHolder changes.

Thanks for the patch, will commit this after A23 is released. Sorry for the delay.

@vladislavbelov Could you commit this after A23 is released?

At the moment I'll focus on sane behaviour regardless of ownership, it's fairly easy to add it in later.

you've missed a static_cast on L87
and some more redundant parens on L111 on the right

Other remarks.

Some more comparable screenshots. The fixed version is the one that doesn't look terrible.

With the changes this time -_-

Yup, it appears I'd forgotten to remove some piece of code or something at some point in the diff updates. Fixed.

Inconsistent zeroes.

At the moment it's all units. I've thought about this and I'm not sure how to handle enemy units yet.

Some comments on why svn unitMotion is a pile of hacks that needs to be cleansed with the holy waters of D13.

I'm going to support formations in D13 so drop this for now.

Some footage.
This is SVN: https://www.youtube.com/watch?v=QQC2wrIUHLQ
You can see that the units move around the phalanx formation, the movement around the tree is wonky, and there is a lot of collisions when they return resources.
This is with D13 and D1490: https://www.youtube.com/watch?v=aRfP7lA1m7Y
You can see that the unit can move through the phalanx in a way that looks (imo) somewhat nice, you can see that they clump around the tree much easier, and the ressource shuttling is also easier.
However, you can also see some bugs. Pushing isn't as natural as it could be, and units are oriented all wrong, and there's some jerking, and also formations aren't 100% similar to SVN yet.

And here's some approximate profiling of HierarchicalPathfinder::Update. We might expect that to be slower because of global regions, but it's actually also faster, 1.5 to 2 times faster (the relationship isn't entirely linear).

Some new profiling of MakeGoalReachable, on the following replay (which is just moving a pack of units around on Seleucid Sandbox).
Behaviour was virtually identical between svn and D53's result, not exactly the same as expected but sensibly the same result.

Still think time is better spent reviewing D13. In D13 there's a TODO for LOS, since after more thinking I do think LOS should be handled in unitMotion. UnitMotion will now be able to warn unitAI that the move will fail, and furthermore UnitAI was updated to do range checks in "Timer" instead of relying on Move Completed (which is far more correct).

(I've been working on this, but it's challenging, of course. I wanted to say generally that I think we'll need something like a collision manager. Currently we move one unit at a time, but this leads to unnecessary traffic jams when a group of units are moving in the same direction, for example. So I think it would be better to break movement into two parts, first moving units to their ideal position, flagging potential collisions as we go, and second going through all the potential collisions to see if the units actually collided, and if so try to resolve the collision by pushing or requesting another path or something.)

I have no idea what these perl scripts are doing and there are no comments, so I'd rather we had a new script, but we don't have a lot of conventions on that.

Read this better, there's two things here. On the "remove obstruction" part, see above.

I like this, how do others feel?

Rename "floating" to "CanFloat" wherever relevant and this is good to go imo, thanks for the patch!

Nice bug.

This fixes the squeezing at the very least.