The changes you wanted are wrong and your attitude is wrong. Planning for a hypothetical future "roc" unit? Just let the code do what it was written and tested to do, don't delay it even more because of imaginary features that don't exist yet and may never, and probably shouldn't share the same code even if they did. And you may well cause a performance issue by taking out the timer. Just commit the fucking patch. It works. It has always worked through 3 years of delays and irrelevant changes.

I have had enough. Years of inaction on this obviously necessary bug fix to an extremely annoying part of the game which was created on Jun 20 2017, this now being May 29 2020. Finally a few comments. So I have hope, I spend my time making some requested changes. Then, nothing again. This community is broken. If anyone wants D665, and wants any more changes made, commandeer the patch yourself. But honestly none of these recent changes were functionally necessary anyway, which is another broken part of the community. Even the first revision of this patch, 3 years ago, without the long pathfinder stuff, was fully working and better than the status quo which is still broken. Just commit the shit and make the game work. Your functionally useless whims and preferences should not become time demands on the patch creator.

I renamed to CheckPickupAccessible because it's actually used for siege pickups as well as ship pickups.

rename CheckNearShore to CheckPickupAccessible, and also test the square directly under the ship.

In D665#115525, @wraitii wrote:

Some comments:

• I think you can use global regions to skip the ReachableRegions logic entirely, which should be faster.

You're right. I have changed the patch to do that. When I first wrote the patch, GetGlobalRegion didn't exist.

Use GetGlobalRegion

style changes

style change

Fix LeaveFoundation unpacking behavior. This required some refactoring to avoid code duplication. As a result, all of LeaveFoundation behavior (siege unpacking, ordinary units, units in formation, animals) needs testing now.

split precondition test

LeaveFoundation

style

style changes

style changes

Remove the extra bugfix that was split into D2174.

Update to trunk. Slightly refactor (not a functional difference from the patch before).

In D665#76161, @Imarok wrote:

One easy fix would be that the ship won't move if the units can be garrissoned with the ship not moving.

That's what the patch already does.

In D665#76122, @elexis wrote:

If I understood the patch correctly, the behavior of the patch is that the ship moves to the closest shoreline, which may be on an entirely different island, at least.

Updated to trunk. Took out the std::unordered_set change, because other code in HierarchicalPathfinder.cpp now depends on the set being ordered.

Yeah, it's not that it wasn't tested, it's that the test coverage wasn't good. My method of testing disconnects was to kill -9 the client process, which for some reason results in a longer time before disconnect if applied during game loading with the earlier patch version (~40s) than the method of setting artificial lag with tc. Also tested rejoins, where it was working fine. And we didn't ever manage a test with a real dropping player.

Tested, works. The message is still being sent while the client is on the loading screen, but it's hidden there and causes no problems.

add config option

fix diff

increase timeout to 80s

In practice, this patch allows about 60 seconds of timeout. I'm not sure on exactly why but that's what local testing shows.

Increase loading timeout from 40s to 80s

In D1513#62429, @elexis wrote:

I just got disconnected during the beginning of a mapgen stage, there could hardly be any freeze. Got 100ms ping to the host.
One can join a remote host with one client, then rejoin with a second. One can look at the first screen to find the moment when the server disconnects, long before the loading screen client notices.

Added code to cancel packing/unpacking the unit as necessary when it is issued an attack order, depending on the range to the target. This code is not pretty due to having to work around the way UnitAI handles packing.

In D1520#62201, @aeonios wrote:

Couldn't this be applied equally the other direction? That is, if a unit is unpacked and packing up, and then given an order to attack something within range, to automatically cancel packing?

EDIT: it should also respect halt orders both ways, if it doesn't already.

From what I know about pushing in SC2, stationary units on a task like attacking (or gathering, although that case does not arise in SC2 because gatherers ignore collisions) do not get pushed at all. There are rules about which units can push which other units.

diff context

misc

add periods to comments

Whitespace fix. Also, I'm now confident through local testing that the patch works.

elexis comments and don't change timeout on a local client

remove the mace/ptol template fix from the patch

I think that grouping packed and unpacked catapults together is better than the current behavior. The player can triple click if they want the old behavior. I've noticed in many of my games that I frequently want to retreat *all* catapults because some anti-catapult thing is approaching, and it's a minor annoyance that double clicking doesn't select them all. I don't think wanting to select all packed or all unpacked is as common a use case. If I only wanted to move certain catapults I would drag to select them.

Added a SelectionGroupName for siege towers as well

The other catapult and bolt shooter templates don't have this problem.

Also added a SelectionGroupName to catapults and bolt shooters

They're not related in the code, but the word "overlay" does describe them. They are things that are painted on top of (overlay) the game simulation.

You requested a comment on IRC: I would prefer the "Overlays" category because it's more clear what goes into it. A "Gameplay" category could include more options than those you listed: windowed mode, detailed tooltips, all sound and graphics settings, and even chat notification settings influence gameplay. It's overbroad.

Looks good. Although I don't think it's necessary to have a separate buffer range. If the alert range (140m) doesn't include all the desired garrison buildings, you could just increase the alert range a bit. I think 140m should be enough for all alerted units and buildings.

But you could also manually ungarrison siege into shallow water in that situation, right? Also, it's questionable how many people at the time could actually swim. Particularly, a hoplite in armor would have probably drowned even if he knew how to swim. The more important point is that no unspoiled player is going to expect this behavior or have any idea what happened when some of his units disappeared. If there's a ship with a mix of infantry and cavalry, and he's used to infantry automatically ungarrisoning when ships sink, he's more likely to assume there wasn't enough space to ungarrison or the ship wasn't close enough to shore, than to assume there is some special rule that cavalry can't swim.

Horses can swim. Anyway it only ejects the units if the ship is already at the shore, i.e. when any unit including siege could have just ungarrisoned normally, so it's hard to make a case that they would have to swim.

IMO ships should eject everything on destroy. To only eject some unit types is a hidden mechanic that's not intuitive or easy for the player to figure out, which has little effect on gameplay except when the player expected things to be ejected but they weren't.

I get a conflict applying the patch, can you update to svn?
C binaries/data/mods/public/simulation/components/AlertRaiser.js

rejected hunk @@ -1,162 +1,135 @@

Looks good except for one thing: if you raise an alert and a unit tries to garrison beyond the alert range, and then you cancel the alert before the unit finishes garrisoning but after the unit has walked beyond the alert range, it will continue to garrison. My suggestion would be to filter the list of buildings a unit may garrison in, to exclude buildings located beyond the alert range from the alert raiser. With that change, when you end the alert, you only need to check buildings out to the alert range instead of alert range + search range.

It's not fixed in SDL 2.0.5, we'd need to submit a patch to a future SDL version. Ubuntu 17.04 does come with SDL 2.0.5.

It's hardcoded by SDL in SDL_mouse.c: SDL_double_click_radius = 1. https://fossies.org/linux/SDL2/src/events/SDL_mouse.c There was some discussion on changing the value here: https://forums.libsdl.org/viewtopic.php?p=51852. Perhaps someone could submit a patch to SDL to increase the default radius and add a setter.

On second thought it seems you can't select a mix of own and ally buildings. There is also a justification for keeping unload-all-by-owner and unload-all as separate commands because petra uses unload-all-by-owner.

Account for a couple things:

If a building is captured or destroyed, ejected units will try to find a new building to garrison inside.

That may not be a good thing. I often will set a rally point for a building before it's destroyed, so that the units inside will run away to the desired location when the building is destroyed.

replaced set with unordered_set (this required it to also be changed in many places in HierarchicalPathfinder)

cleanup

Fix an additional (pre-existing) bug discovered in UnitAI where if you give the order to garrison the same unit in two ships in a row, the second ship will not pickup.

vladislav changes

"The bug" in this context is where the ship moves away from the shore when the user wants to put something in it. This specific gameplay issue doesn't need the solution you're describing. Finding a pickup location is separate from shoreline detection, and a complete pickup solution will need both (see below), so this patch is a step forward in any case. I also want to note that mimo only asked for connectivity based shoreline detection, which we now have, so let's move forward on that.

I have updated the patch so that it uses the hierarchical pathfinder to determine whether the ship is on a shore that the unit to be picked up can reach.