-
-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[New Feature] Autoload Command Shortcut #1452
base: develop
Are you sure you want to change the base?
Conversation
(Falsely include irrelevent file that belongs to another feature) |
I solved 3, 4, and 5. See below for details. |
When I select an amphibious transport on water and units on land at a same time and press the auto load button, the amphibious transport will move ashore all the way to the land unit and stop there. The land unit doesn't get into the transport. However, this is exactly the same behavior when I call a land unit to get into an amphibious transport on water, so this isn't really an issue. The implementation is mindful. It directly gives enter order to selected units so everything will behave the same as if the player explicitly gave an enter order. It is very convenient to use, even I can now enjoy playing the Allies. |
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
Hello, made Ares tag support for you, and more. Effects:
Maybe we have to detect if Ares is present before we take compatibility with Ares tags? |
have you test in multiplayer game? |
Wow, thanks for your work. I am not famillar with other features on transport loading and you help with the corner cases. For ares tags, maybe we could have the "original" default value for them. For example, |
Ares tags support and smarter unit size logic and more
Not yet, trying to find someone for help. |
Co-authored-by: Kerbiter <[email protected]>
Yes, I defaulted every vehicle to allow maual enter and to count passengers by size. |
minor adjustments
@chaserli Hello, this pull request seems ready for review. Maybe take a look when you have time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, lemme fix some of the style stuff quickly.
docs/User-Interface.md
Outdated
- Larger passengers are loaded first, and transports with smaller size limits are used first. | ||
- At a given unit size and size limit, passengers will be diversely distributed into transports if possible. | ||
- Ares `Passengers.Allowed=` and `Passengers.Disallowed=` are taken into account. | ||
- It also supports Bio Reactors and Tank Bunkers. Select valid candidates and multiple said buildings while pressing Shift, then press the auto load hotkey, the units will be distributed among these buildings and will be ordered to enter them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Soviet Battle Bunkers and other structures that you own and can select and can have garrisons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Garrisonable structures are currently not supported. Neutral structures and owned troops can't be selected at a same time. Also can't read the user's intention when a bio reactor, a battle bunker, and a group of conscripts are selected at a same time. Maybe we need another shortcut key for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can't read the user's intention when a bio reactor, a battle bunker, and a group of conscripts are selected at a same time.
Everyone who can fit in bio reactor but not the garrison goes to bio reactor, everyone else is spread between bunkers and reactors. I think the situation when battle bunkers and reactors will be selected is rather rare so there's no need to do much thought about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I will add such support later. However since neutral structures still can't be selected with owned troops at a same time, this will only ease the loading of battle bunkers, which is always owned by a player even when not garrisoned.
Co-authored-by: Kerbiter <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant sanity checks such as misuse of abstract_cast and all kinds of sanity check on xxExt are not the most significant code style issues in the current code.
I have the impression that the entire selection-filtering is somehow repeated. I can think of 2 ways: either you just let the game tell which cursor to use and select the "load onto vehicle" cursors among them. Or just handle the network event directly.
If you don't fully understand how network events are handled, you can check the implementation of the virtual function you've chosen. Methods in EventClass.h in yrpp is also updated to handle them directly.
@chaserli it will be good to see the actual explanations of why do you think something is wrong (like the cast thing) and how and it should be done properly. Code reviews should be done so the author can learn and grow, saying "it is wrong" without explaining when you know the answer is not productive. |
Garrisonable structure support & extract redundancy as inline function
src/Commands/AutoLoad.cpp
Outdated
// If return value <= 0 then the building can't be a "transport". | ||
inline static const int GetBuildingPassengerBudget(BuildingClass* pBuilding) | ||
{ | ||
auto pBuildingType = abstract_cast<BuildingTypeClass*>(pBuilding->GetTechnoType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly use pBuilding->Type
.
src/Commands/AutoLoad.cpp
Outdated
else if (pPassenger->WhatAmI() == AbstractType::Unit) | ||
{ | ||
// Tank Bunker | ||
return pPassenger->GetTechnoType()->Turret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed a lot of checks. See sub_70FB50.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果你不想仔细检查每个可能的进入关系是否成立的话,可以试试使用MouseOverObject。它会返回一个Action,只要返回的Action是Enter那应该就是可以进入的。不过这个函数会进行很多很多判断,如果你要用的话最好实际测试一下会不会造成卡顿。
If you don't want to carefully check whether each possible entry relationship is established, you can try using MouseOverObject. It will return an Action. As long as the returned Action is Enter, it should be possible to enter. However, this function will make many, many checks. If you decide to use it, it is best to actually test whether it will cause lag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried before you say, didn't work. Most objects don't yield Action::Enter
when selected by the player, only Bio Reactors do. Meanwhile, if we simply invoke ObjectClickedAction
, most objects can be entered when Action::Enter
is given, even when the object is selected by the player, only Tank Bunkers can't be entered this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed a lot of checks. See sub_70FB50.
As far as I read from the assembly, it seems hovered units, parasited units, and those without a "turret changer" weapon, are never eligible to enter a Tank Bunker, right?
I don't know what is a turret changer. The rest can be added to the filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly reference that function by using reinterpret_cast
. The use cases can be found in the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, didn't know that, thank you.
src/Commands/AutoLoad.cpp
Outdated
if (pTransport->WhatAmI() == AbstractType::Building) | ||
{ | ||
auto pBuilding = abstract_cast<BuildingClass*>(pTransport); | ||
auto pBuildingType = abstract_cast<BuildingTypeClass*>(pBuilding->GetTechnoType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pBuilding->Type
Reselected after deselected
@chaserli Hello, redundancy have been migrated to inline functions, so it doesn't look as redundant as before. Maybe you can take a look again when you have time. Also I don't know what's wrong with the "misuse" of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that you understand the abstract_cast
. But I found some parts that can be omitted to avoid repeating check.
src/Commands/AutoLoad.cpp
Outdated
return pPassenger->GetTechnoType()->Turret | ||
&& pPassenger->GetTechnoType()->Bunkerable | ||
&& pPassenger->GetTechnoType()->SpeedType != SpeedType::Hover | ||
&& !abstract_cast<FootClass*>(pPassenger)->ParasiteEatingMe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!static_cast<FootClass*>(pPassenger)->ParasiteEatingMe
src/Commands/AutoLoad.cpp
Outdated
} | ||
else if (pTransport->WhatAmI() == AbstractType::Building) | ||
{ | ||
auto pBuilding = abstract_cast<BuildingClass*>(pTransport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if (auto pBuilding = abstract_cast<BuildingClass*>(pTransport))
src/Commands/AutoLoad.cpp
Outdated
// the check of MCing is redundant here, see inline function "CanBeBuildingPassenger". | ||
return pPassenger->WhatAmI() == AbstractType::Infantry | ||
&& !pPassenger->IsMindControlled() | ||
&& abstract_cast<InfantryTypeClass*>(pPassenger->GetTechnoType())->Occupier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static_cast<InfantryTypeClass*>(pPassenger->GetTechnoType())->Occupier
src/Commands/AutoLoad.cpp
Outdated
{ | ||
if (pTransport->WhatAmI() == AbstractType::Unit) | ||
{ | ||
auto pType = pTransport->GetTechnoType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto pUnit = abstract_cast<UnitClass*>(pTransport))
{
auto pType = pUnit->Type;
...
}
src/Commands/AutoLoad.cpp
Outdated
{ | ||
if (pTransport->WhatAmI() == AbstractType::Building) | ||
{ | ||
auto pBuilding = abstract_cast<BuildingClass*>(pTransport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto pBuilding = abstract_cast<BuildingClass*>(pTransport))
src/Commands/AutoLoad.cpp
Outdated
// Detect enterable buildings. | ||
if (pTechno->WhatAmI() == AbstractType::Building) | ||
{ | ||
auto pBuilding = abstract_cast<BuildingClass*>(pTechno); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto pBuilding = abstract_cast<BuildingClass*>(pTechno))
src/Commands/AutoLoad.cpp
Outdated
{ | ||
// If by size then get the actual size of the potential passenger. | ||
// Otherwise, every potential passenger counts as size 1. | ||
passengerSize = static_cast<int>(abstract_cast<TechnoClass*>(pPassenger)->GetTechnoType()->Size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is determined that pPassenger
is a TechnoClass*
, you can replace the redundant abstract_cast
with reinterpret_cast
Thank you for your detailed review. I appreciate. |
For selected units, pair the transport with infantry/vehicle.