feat(ui): Lock ships to prevent accidental selling #9844
Conversation
bene-dictator
added
enhancement
|
I could also make the ships in the player info display have a little indicator if they're favourited, if people want that. (done) |
|
I'd add instructions on how to unmark a ship to the sell dialog (or to the shipyard help dialog), because people may get confused if they favorited the ship accidentally. Also iirc American spelling is preferred generally ("favorite"). |
|
I'd prefer to use lock/unlock terminology instead of favoriting. Also, is there any reason to have sold ship hulls be repurchasable in the shipyard if this mechanic exists? (#9085) Being able to see sold ship hulls does give access to otherwise invisible ship descriptions, but I'd rather we resolve the issue of invisible ship descriptions via other means, because otherwise we'd be incentivizing selling your ship just to see its description. |
|
Also,
I'm a fan of the idea of locking ships from being sold, but I think the method of locking a ships should be less opaque. How we'd have a less opaque method of locking ships though, I'm not sure. Perhaps we should add new buttons between the currently selected ship's info and escorts list, or above the escorts list. Bonus points if we can think of anything else that's currently hidden that could be a button so that a lock button isn't all by itself. Should it also be possible to lock outfits? Locked outfits wouldn't be able to be sold just as with locked ships, but selling a ship with a locked outfit would move the outfit to storage instead of preventing sale of the ship. (Since outfits are fungible, locking an outfit would actually mean locking all outfits of that type; e.g. locking a jump drive would mean no jump drive can be sold.) |
Park/unpark from the shop, perhaps? 🤔 |
|
Some sort of a dropdown context menu would be a good place to put this if we want to avoid having too many buttons. And there's already a similar request (#8823, a context menu for sorting ships). |
Koranir
changed the title
I think that's a bit out of scope for this PR, maybe someone can do a follow-up that uses drop-downs or something to clean up the whole shop panel that this kind of feature will get included in.
Currently I'm using UUIDs to check for locked ships, so it's not that feasible to add support for locking outfits. Also, it's not quite as high of a priority problem as outfits can be bought back into cargo quite easily. |
|
Regarding #9085 , it'd allow for the exploit that outfits that can't simply be taken out of ships they're in and put in others, could be achieved by virtue of selling the ship, buying the outfit into whatever ship you want, and then buying the original ship back, ie with the Arfecta Cloaking Devices. So I think this PR is the better approach to preventing the accidental selling of ships. |
|
Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>
|
@Koranir Thank you very much for opening a PR to address my issue. I have some thoughts about the Ship Locking UI I'd like to share.
For the Ship info menu,I think the As inside the Shipyard,I made a crude example base on Koranir's solution.
|
Also instead of adding more hot keys like The following figure shows a selected ship is current unlocked.So there is a button besides
And now the following figure shows that the selected ship is now locked.Player can click the button to unlock the ship.
The underline I know it's a pain code-wise,Sorry about this and thank you all again. |
|
I tested this and have a couple notes.
|
I agree with this, but it seems like a feature that can be added in a later PR, I want to focus on getting an MVP into master as soon as possible.
I'll work on this later
People should be locking their important ships anyways, and I've sold my flagship to get enough cash to upgrade to another ship before and I don't want to remove that capability.
Not in scope for this PR, maybe open up an issue if their isn't one already (perhaps there could be a button in the ship info panel?) |
I'm not sure it's good to add the whole fleet management UI to the shipyard - that's what the Player Info panel is for. Locking is an exception as it's directly related to selling ships. |
|
When #9825 gets merged, we could add a note on the tooltip if the ship is locked. |
|
You both misunderstood my last request involving name change.
Proposed:
I was not suggesting to include the whole ship info UI into the shipyard. However, I feel, changing the name should be available from the shipyard UI. An alternative would be to allow hot key "i" to work in the shipyard. I just assumed that because you where already working in this area, it would be a quick fix. If I was wrong, I apologize, and will save this request for another time. |
Minimum Viable Product. I.e., the least amount of functionality while still being considered viable. |
data/_ui/help.txt
Outdated
| @@ -150,6 +150,7 @@ help "shipyard" | |||
| `Here, you can buy new ships.` | |||
| `Use your scroll wheel or click and drag, to scroll the view.` | |||
| `As in the trading and outfitter panels, you can hold down Shift, Control, or Alt to buy 5, 20, or 500 ships at once, or multiple keys for larger amounts.` | |||
| `Clicking a ship while holding Alt will lock it, preventing it from being sold. Alt+clicking will unlock it again.` | |||
There was a problem hiding this comment.
| `Clicking a ship while holding Alt will lock it, preventing it from being sold. Alt+clicking will unlock it again.` | |
| `Selecting a ship while holding the Alt key will lock it, preventing it from being sold. Alt+clicking a second time will unlock it.` |
I'd rather say it like that personally.
There was a problem hiding this comment.
"Clicking a ship while holding the Alt key will lock it, preventing it from being sold. Alt+clicking a second time will unlock it .`
IMO is even clearer.
yes
Yes, why is it not?
There is already a dialog that asks for confirmation before selling, and if the player is worried about selling a particular ship, they can always double check before actual selling. I don't even see how locking a ship would even be useful. Wouldn't you want to lock every single one of your ships? In that case, if you want to sell you might unlock a ship you don't want to sell so you have the same problem. Or, if you don't lock every ship, then either you actually forget to lock the ships you want to protect, or if you don't forget, then you might as well not sell them because you're paying attention. At least IMO. |
| GetUI()->Push(new Dialog(message)); | ||
| return; | ||
| } | ||
| else if(all_of(playerShips.begin(), playerShips.end(), [this](Ship *ship) |
There was a problem hiding this comment.
I'd just exclude all locked ships and if the final ship count is 0, the dialog would be displayed. Because currently, the confirmation message gives you the value of all ships selected to be sold, not only those that will be actually sold. Which may be misleading if you attempt to sell a valuable locked ship along with some unlocked ships.
There was a problem hiding this comment.
I would do something like:
int selectedCount = playerShips.size();
vector<shared_ptr<Ship>> toSell;
for(const auto &it : playerShips)
if(!player.UuidLocked(ship->UUID()))
toSell.push_back(it->shared_from_this());
if(toSell.empty())
{
string message("Could not sell the ");
if(selectedCount == 1)
message += playerShip->Name() + ", as it is";
else
message += "selected ships, as they are all";
message += " marked as locked.\n(Alt+click a ship's icon to unlock the ship.)";
GetUI()->Push(new Dialog(message));
return;
}
int count = toSell.size();
int initialCount = count;
int maxList = (count == selectedCount) ? MAX_LIST : MAX_LIST - 3;
string message;
if(!toStorage)
message = "Sell the ";
else if(count == 1)
message = "Sell the hull of the ";
else
message = "Sell the hulls of the ";
if(count == 1)
message += playerShip->Name();
else if(count <= maxList)
{
auto it = toSell.begin();
message += (*it++)->Name();
--count;
if(count == 1)
message += " and ";
else
{
while(count-- > 1)
message += ",\n" + (*it++)->Name();
message += ",\nand ";
}
message += (*it)->Name();
}
else
{
auto it = playerShips.begin();
message += (*it++)->Name() + ",\n";
for(int i = 1; i < maxList- 1; ++i)
message += (*it++)->Name() + ",\n";
message += "and " + to_string(count - (maxList- 1)) + " other ships";
}
int64_t total = player.FleetDepreciation().Value(toSell, day, toStorage);
message += ((initialCount > 2) ? "\nfor " : " for ") + Format::CreditString(total) + "?";
int locked = selectedCount - initialCount;
if(locked)
{
if(locked == 1)
message += "\nSome ships are";
else
message += "\nOne ship is";
message += " marked as locked and will not be sol.\nAlt+click a ship's icon to unlock the ship.";
}
if(toStorage)
{
message += " Any outfits will be placed in storage.";
GetUI()->Push(new Dialog(this, &ShipyardPanel::SellShipChassis, message, Truncate::MIDDLE));
}
else
GetUI()->Push(new Dialog(this, &ShipyardPanel::SellShipAndOutfits, message, Truncate::MIDDLE));This may require some fine turning.
The main point is to add sufficient friction against accidentally selling a ship. While the confirmation dialog does add some friction, it's very easy to get past; it adds only a single extra click/button press to selling a ship. While you could look at the name of the ship you're selling, that doesn't work if you're mass selling ships and there you get the "And X other ships" bit. Say you accidentally grab one too many ships in your selection to sell, the name doesn't appear in the first 10 or however many display before it stops listing names, and you accept the dialog, then you've lost a ship you may not have wanted to sell and there's no recourse to getting it back aside from loading an older save. You could perhaps repurchase the ship or recapture it, but that can take a considerable amount of effort, and some ships are entirely unique and couldn't be reobtained if lost. Being able to "lock" ships to prevent yourself from selling them until you "unlock" them is one way to avoid the accidental sale of a ship, and is actually a mechanic you can find in other games. A game that I play that uses this locking feature is Destiny. When you dismantle (sell) your gear in Destiny, you have to hold a button/key down for a few seconds; this serves a similar purpose as our confirmation dialog. But once an item is dismantled, there's no way of getting it back (at least in most cases). And like it's not inconceivable that someone could accidentally sell one of their ships in Endless Sky, it's not inconceivable that someone could accidentally dismantle an item they didn't intend to in Destiny. Destiny's solution to this is giving players the ability to lock items, preventing their dismantling until they are unlocked. This adds an extra step (i.e. extra friction) to dismantling items, almost entirely preventing the accidental loss of items. Making selling a ship go from two button presses (sell + confirm) to three (unlock + sell + confirm) might not sound like a lot, but it's proven to work elsewhere. (This same friction doesn't need to be added to outfits because an outfit that's been sold can immediately be repurchased, and we've already introduced extra friction to the loss of unique outfits via #9112.) Perhaps not everyone will use this feature, but I can imagine it being a helpful QoL feature for many. At the very least it's not something that would get in the way for those who don't want to use it. |
| else if(child.Token(0) == "locked ships") | ||
| for(const DataNode &grand : child) | ||
| if(grand.Size() >= 1) | ||
| lockedIds.emplace_back(EsUuid::FromString(grand.Token(0))); |
There was a problem hiding this comment.
I don't think it's possible to have a node with no tokens.
| else if(child.Token(0) == "locked ships") | |
| for(const DataNode &grand : child) | |
| if(grand.Size() >= 1) | |
| lockedIds.emplace_back(EsUuid::FromString(grand.Token(0))); | |
| else if(child.Token(0) == "locked ships") | |
| for(const DataNode &grand : child) | |
| lockedIds.emplace_back(EsUuid::FromString(grand.Token(0))); |
|
|
||
| void PlayerInfo::UnlockUuid(const EsUuid &uuid) | ||
| { | ||
| lockedIds.erase(find(lockedIds.begin(), lockedIds.end(), uuid)); |
There was a problem hiding this comment.
This could try to erase the end iterator, which is undefined behaviour.
| GetUI()->Push(new Dialog(message)); | ||
| return; | ||
| } | ||
| else if(all_of(playerShips.begin(), playerShips.end(), [this](Ship *ship) |
There was a problem hiding this comment.
I would do something like:
int selectedCount = playerShips.size();
vector<shared_ptr<Ship>> toSell;
for(const auto &it : playerShips)
if(!player.UuidLocked(ship->UUID()))
toSell.push_back(it->shared_from_this());
if(toSell.empty())
{
string message("Could not sell the ");
if(selectedCount == 1)
message += playerShip->Name() + ", as it is";
else
message += "selected ships, as they are all";
message += " marked as locked.\n(Alt+click a ship's icon to unlock the ship.)";
GetUI()->Push(new Dialog(message));
return;
}
int count = toSell.size();
int initialCount = count;
int maxList = (count == selectedCount) ? MAX_LIST : MAX_LIST - 3;
string message;
if(!toStorage)
message = "Sell the ";
else if(count == 1)
message = "Sell the hull of the ";
else
message = "Sell the hulls of the ";
if(count == 1)
message += playerShip->Name();
else if(count <= maxList)
{
auto it = toSell.begin();
message += (*it++)->Name();
--count;
if(count == 1)
message += " and ";
else
{
while(count-- > 1)
message += ",\n" + (*it++)->Name();
message += ",\nand ";
}
message += (*it)->Name();
}
else
{
auto it = playerShips.begin();
message += (*it++)->Name() + ",\n";
for(int i = 1; i < maxList- 1; ++i)
message += (*it++)->Name() + ",\n";
message += "and " + to_string(count - (maxList- 1)) + " other ships";
}
int64_t total = player.FleetDepreciation().Value(toSell, day, toStorage);
message += ((initialCount > 2) ? "\nfor " : " for ") + Format::CreditString(total) + "?";
int locked = selectedCount - initialCount;
if(locked)
{
if(locked == 1)
message += "\nSome ships are";
else
message += "\nOne ship is";
message += " marked as locked and will not be sol.\nAlt+click a ship's icon to unlock the ship.";
}
if(toStorage)
{
message += " Any outfits will be placed in storage.";
GetUI()->Push(new Dialog(this, &ShipyardPanel::SellShipChassis, message, Truncate::MIDDLE));
}
else
GetUI()->Push(new Dialog(this, &ShipyardPanel::SellShipAndOutfits, message, Truncate::MIDDLE));This may require some fine turning.
warp-core
added
the
waiting on OP
|
Maybe a padlock icon instead of the asterisk? I wouldn't know what the asterisk meant until I read it in your code. |
|
Closing as orphaned, as this has unresolved comments from March. Feel free to reopen this PR once the comments are resolved. |
tibetiroka
added
orphaned
Feature
This PR addresses the bug/feature described in issue #9833
Summary
This PR lets you Alt+Click a ship in the shipyard's sidebar to lock it, preventing you from selling it inadvertently while bulk selling or giving a warning if trying to sell it individually.
Screenshots
(Middle two are locked)
