Skip to content
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

Impossible/invalid orders aren't marked as such in the order list #7972

Closed
James103 opened this issue Feb 4, 2020 · 10 comments · Fixed by #8516
Closed

Impossible/invalid orders aren't marked as such in the order list #7972

James103 opened this issue Feb 4, 2020 · 10 comments · Fixed by #8516
Milestone

Comments

@James103
Copy link
Contributor

James103 commented Feb 4, 2020

Version of OpenTTD

d7a928a
(Based on #5213, meaning that it was also broken in 1.2.1)

Expected result

If you clone a vehicle whose orders are impossible to make (e.g. a bus going to a truck-only station), the orders will be copied into the new vehicle. Attempting to share/unshare order lists with impossible orders works normally.

Actual result

If you clone a vehicle whose orders are impossible to make (e.g. a bus going to a truck-only station), the orders will not be copied into the new vehicle and no error message is displayed. Attempting to share/unshare order lists with impossible orders fails.

Steps to reproduce

  1. Buy a new bus.
  2. Build a bus station and name it "A".
  3. Build another station that has both a bus and truck component, and name it "B".
  4. Order the bus to go to both stations.
  5. Demolish any bus stops with truck stops attached, leaving only the truck stops.
  6. Look at the order list. The order "Go to B" is not marked as invalid.
  7. Attempt to copy the vehicle. None of the orders are copied and no warning is shown.
  8. Attempt to copy the vehicle with order sharing. This works normally, at first.
  9. Attempt to share or unshare the impossible orderlist. This will fail.
@LordAro
Copy link
Member

LordAro commented Feb 9, 2020

I think the actual issue here is that the invalid orders (bus to truck stop) are not properly marked as such in the order list. (e.g. like when the station is removed entirely)

It's perfectly reasonable to not copy invalid orders (when not sharing), and it mostly makes sense that the invalid order is copied when sharing.

You can't unshare the list as that would create an invalid order, which isn't allowed. You can delete the invalid order (which should be displayed as such), and then you can successfully unshare.

@James103 James103 changed the title Impossible orders silently fail to be copied and have their sharing settings fully protected. Impossible/invalid orders aren't marked as such in the order list Jun 2, 2020
@James103
Copy link
Contributor Author

James103 commented Jun 2, 2020

Edited title and description to expose the actual issue more (it still occurs in 1.10.2).

@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain
Copy link
Member

This happens for more than only buses vs trucks. It basically happens for everything :) Pointing a train to a train-station, connect a bus-station, remove the train-station, same issue :)

@TrueBrain
Copy link
Member

TrueBrain commented Jan 7, 2021

A solution is a bit more tricky than I was expecting. Initially, I was thinking doing the same as when removing stations: just make the order invalid.

But what if you remove the Bus station to place it on a new spot seconds later. All orders would already been invalidated (and they will not recover). So that is far from ideal.

Using a timeout, as with stations, is also difficult, as there is no visual feedback on that.

Guess we could mark it red or something (and postfix it with invalid?), but leave the order there? But that still leaves the copy problem, which just silently fails.

@James103
Copy link
Contributor Author

James103 commented Jan 7, 2021

Guess we could mark it red or something (and postfix it with invalid?), but leave the order there? But that still leaves the copy problem, which just silently fails.

As long as there's some way to see that the order is invalid without overwriting the order itself with Invalid Order, I like it.

Maybe also display a warning when copying impossible orders in order to allow players to better resolve such orders? For example: "Vehicle contains invalid orders" (though there's also "Vehicle can't go to all stations").

@TrueBrain
Copy link
Member

Copying no longer silently fails with #8515, so that would open the way to just mark the order as invalid (instead of removing it). Still not sure it is the best thing to do :)

@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 7, 2021

I'm having a few thoughts about order lists, that may or may not be related to the problem here:

  1. there exists some feature that periodically checks order lists, and issues a news message, when things are "wrong", however, it's always been totally unclear what this actually checks.
  2. there are "invalid orders" when a station is completely removed, we might want to extend that to orders that cannot be fulfilled because of incompatible stations, but where do we want to stop?
    • Vehicle is a bus, but station has no bus stop
    • Vehicle is an articulated bus, but station has no drive-through bus stop
    • Vehicle is a monorail, but station has only maglev tracks
  3. if we are somehow marking entries in the order list in red for any of these reasons, how do we give a hint about what exactly is wrong? order lists are already a heavily overloaded GUI that is in dire need of untangling.

@TrueBrain
Copy link
Member

TrueBrain commented Jan 7, 2021

image

Is what I came up with for now. It complements 1), as they both now show the same information. It works for any station the vehicle is not compatible with, so all of your 2), and I think the text is clear enough to solve 3) too. Well, partially .. it is still bloated .. you can now get this:

image

or this:

image

:D

@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 7, 2021

that your second screenshot is much wider than the first is proof that 3) is an actual problem :)

TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Jan 7, 2021
…our vehicle

Before it was shown as a normal order, but the vehicle was skipping
it. This was rather unclear to the user. Now it is red and contains
text with some hints what is going on.

The text is prefixed rather than postfixed, as we have many
postfixes already.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Jan 7, 2021
…our vehicle

Before it was shown as a normal order, but the vehicle was skipping
it. This was rather unclear to the user. Now it is red and contains
text with some hints what is going on.

The text is prefixed rather than post-fixed, as we have many
post-fixes already.
@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 7, 2021

For reference, an order list in a competitor product that shall remain unnamed :)

Screenshot_20210107_164233

TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Jan 7, 2021
…our vehicle

Before it was shown as a normal order, but the vehicle was skipping
it. This was rather unclear to the user. Now it is red and contains
text with some hints what is going on.

The text is prefixed rather than post-fixed, as we have many
post-fixes already.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Jan 8, 2021
…our vehicle

Before it was shown as a normal order, but the vehicle was skipping
it. This was rather unclear to the user. Now it is red and contains
text with some hints what is going on.

The text is prefixed rather than post-fixed, as we have many
post-fixes already.
TrueBrain added a commit that referenced this issue Jan 8, 2021
…icle (#8516)

Before it was shown as a normal order, but the vehicle was skipping
it. This was rather unclear to the user. Now it is red and contains
text with some hints what is going on.

The text is prefixed rather than post-fixed, as we have many
post-fixes already.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants