-
-
Notifications
You must be signed in to change notification settings - Fork 958
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
Fix #7561: Remove assumption between power and cost #7573
Conversation
The use of float here should be safe, I think, since this is a UI-only thing? |
Yes, but I'm not sure what problem is being solved, Maybe some examples? |
It wouldn't sort properly because some vehicles had more power than running cost. Just check with any set that adds vehicles with more power than cost. Let me find an easy example. |
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, please squash your PR down to one commit when you're done.
As you can tell i am kinda new to contributing to projects in github, and more so to this one. How would i go about squashing my commits? I don't want to make a mess of my git commits. |
there's different ways to do this. this is just one:
|
1cf644b
to
817aaa5
Compare
It is done. Is there anything i am missing? |
Hmm, ordering of wagons seems odd with this PR. |
What do you mean? Since most don't have power they should get ordered by ID. Oh, i see. Yeah without the PR it defaults to sort by running cost. I'll check how to make it behave like before for wagons. |
Ideally make it behave as it already does, just with correct power/running cost sorting first? You can easily fix the current power/running cost sorting by multiplying running cost by 1000 or so. No other changes, nor floats, needed... |
This should work too. I checked quickly and sorting looks like previous. Let me know if it's not sufficient. |
53f2c8e
to
0f8ec4d
Compare
Hi. It's been a while since i checked this. Is there anything missing for this to get approved? I recall fixing every detail. |
Removed assumption that running cost was higher than power so that it works properly in all cases. Also made it so that in cases where both engines had same power/running cost value it defaulted to the EngineNumberSorter as previously done.