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

Fix #7561: Remove assumption between power and cost #7573

Merged
merged 1 commit into from Oct 21, 2019

Conversation

@dorobouNeko
Copy link
Contributor

@dorobouNeko dorobouNeko commented May 5, 2019

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.

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented May 5, 2019

The use of float here should be safe, I think, since this is a UI-only thing?

@PeterN
Copy link
Member

@PeterN PeterN commented May 5, 2019

Yes, but I'm not sure what problem is being solved, Maybe some examples?

@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 5, 2019

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.

src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 5, 2019

sorter
Here you can see that according to the sorter:
4602/1915 > 5871/2168
aprox.
2.4 > 2.7
Which is not true at all.

@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 5, 2019

2019-05-05-190815_700x763_scrot

Same list sorted properly.

src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
PeterN
PeterN previously requested changes May 6, 2019
Copy link
Member

@PeterN PeterN left a comment

Also, please squash your PR down to one commit when you're done.

src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 6, 2019

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.
Something like what is described here?
https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented May 6, 2019

there's different ways to do this. this is just one:

  1. if you want to make sure you can go back to the separate commits, make a git tag (tags are by default local to your private repo, you should not push tags like this to a public repo)
  2. git rebase -i, and select all the commits to merge with the first one, update the commit message as appropriate
  3. git push -f to update your PR

@dorobouNeko dorobouNeko force-pushed the fix-sorter branch 2 times, most recently from 1cf644b to 817aaa5 May 6, 2019
@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 8, 2019

Also, please squash your PR down to one commit when you're done.

It is done. Is there anything i am missing?

@PeterN
Copy link
Member

@PeterN PeterN commented May 10, 2019

Hmm, ordering of wagons seems odd with this PR.

@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 10, 2019

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.
Although i am not sure. There is already a running cost sorting. What should i do?

@PeterN PeterN dismissed their stale review May 10, 2019

Resolved

@PeterN
Copy link
Member

@PeterN PeterN commented May 13, 2019

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...

@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented May 14, 2019

This should work too. I checked quickly and sorting looks like previous. Let me know if it's not sufficient.

src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
@dorobouNeko
Copy link
Contributor Author

@dorobouNeko dorobouNeko commented Aug 14, 2019

Hi. It's been a while since i checked this. Is there anything missing for this to get approved? I recall fixing every detail.

LordAro
LordAro approved these changes Oct 9, 2019
@LordAro LordAro merged commit cbefc1d into OpenTTD:master Oct 21, 2019
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants