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

Add: BuildVehicleSmartGUI #7653

Closed
wants to merge 3 commits into from

Conversation

imcasper
Copy link

When the amount of types locomotives and wagons becomes unrealistic a lot ... "Vehicle Build Smart GUI” will come to the rescue. Filters will allow you to choose the cheapest locomotive for a given train weight, preferred speed, number of slopes, types of cargo. Also, you can easily pick up the most capacious wagons for this price. Or for example the parameter: "capacity / length" - when the limiting factor is the length of the rail platform.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jul 13, 2019

your commit messages should adhere to our style, most notably they should begin with something like "Codechange:", "Add:" or "Feature:" (depending on how visible the change is to a user)

@imcasper imcasper changed the title Build vehicle smart gui Add BuildVehicleSmartGUI Jul 13, 2019
@imcasper
Copy link
Author

imcasper commented Jul 13, 2019

Thank. I'm here for the first time. Therefore, I try to understand what and how.

PS I renamed the brunch. However, the exact same error.

@imcasper imcasper changed the title Add BuildVehicleSmartGUI Add: BuildVehicleSmartGUI Jul 13, 2019
Copy link
Author

@imcasper imcasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request name changed.

@michicc
Copy link
Member

michicc commented Jul 13, 2019

It's not about the branch name, it's about your commit messages.
Details at https://wiki.openttd.org/Coding_style#Commit_message

@nielsmh
Copy link
Contributor

nielsmh commented Jul 13, 2019

You'll have to use git rebase -i to modify your commit messages, then force-push to your branch to update the commit messages, so they follow the requirements.

Additionally: Don't modify the Visual Studio project files directly, you should modify the source.list file to include the new files, then run the projects/generate.vbs script (or the non-vbs version if you use Mingw, WSL, Cygwin, or similar) to re-generate all the project files.

You also need to run the src/script/api/generate_widget and src/script/api/squirrel_export scripts, in that order, when you add/change/remove any widget numbers.

#include <map>

template <class Key, class Value>
using Map = std::map<Key, Value>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of this file, or its contents
aliasing std::map to Map is just pointless obfuscation to save 5 characters, and GetOrDefault doesn't actually seem to be used anywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like simple and clear names. I believe that in this case it is obvious that Map is a key-value store. And the details of the implementation should not be important.
In addition, I do not force anyone to connect and use the library. I have big plans, and I want to have a handy toolkit that will allow me to focus on the idea, and not on the task of finding an item, storing lists, keys, values, and so on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It goes against the coding conventions in the rest of the project, and in fact we have recently moved away from using custom types, to using STL types directly. Please remove this file.

@imcasper
Copy link
Author

I am at a dead end. The last report indicates errors in the trailinig spaces in the comments that I fixed in the latest version (commited and pushed).

@nielsmh
Copy link
Contributor

nielsmh commented Jul 14, 2019

You need to squash/fixup your commits so there aren't any "bad" ones at all, the commit checker looks at each commit individually, not just the final state of the code.

@imcasper
Copy link
Author

I understood. Thank. When everything is set up, then I will release the version.

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went over your code for code style rules, haven't tested yet.

I'm not sure why GitHub includes changes from already-merged PRs in the diff it shows. Can you check your branch is actually rooted properly? (git rebase master)

#include <map>

template <class Key, class Value>
using Map = std::map<Key, Value>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It goes against the coding conventions in the rest of the project, and in fact we have recently moved away from using custom types, to using STL types directly. Please remove this file.

@@ -1000,6 +1000,26 @@ class ScriptWindow : public ScriptObject {
WID_BV_SHOW_HIDE = ::WID_BV_SHOW_HIDE, ///< Button to hide or show the selected engine.
WID_BV_BUILD_SEL = ::WID_BV_BUILD_SEL, ///< Build button.
WID_BV_RENAME = ::WID_BV_RENAME, ///< Rename button.
// build vehicle smart:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whole-line comments are usually marked with /* */, the simple double-slash style is reserved for end-of-line comments with very short explanations.

src/train.h Outdated
*/
inline uint16 GetWeight() const
{
return GetCustomWeight(this->cargo.StoredCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always use this-> for member functions and variables. Same applies to more functions below.

Suggested change
return GetCustomWeight(this->cargo.StoredCount());
return this->GetCustomWeight(this->cargo.StoredCount());

STR_CARGO_FILTER_LABEL_TOOLTIP :{BLACK}Cargo filter
STR_TRAIN_SPEED_LABEL :{BLACK}Train speed:
STR_TRAIN_SPEED_EDITOR :{ORANGE}{VELOCITY}
STR_TRAIN_SPEED_TOOLTIP :{BLACK}Preffered train speed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: "preferred" (same mistake in several more lines)

STR_BUILD_VEHICLE_DETAIL_TOOLTIP :{BLACK}Extended options for comfortable unit select
STR_BUILD_VEHICLE_AUTO_TRAIN :{BLACK}Take the train parameters from the from
STR_BUILD_VEHICLE_AUTO_TRAIN_TOOLTIP :{BLACK}Take parameters from the first train from the depot (maximum weight, available speed, train length)
STR_BUILD_VEHICLE_FILTER :{BLACK}Use filters...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTTD generally does not use ellipses to indicate whether a button/menu will open a new window. Just leave it out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I do not understand what you mean. Button shows / hides the panel with additional filters. The point is to see and adjust filters only if necessary. In a vanilla game, for example, this is usually not necessary.

}

bool IsSmartMenu() {
return (window_desc->nwid_length == sizeof _nested_build_vehicle_smart_widgets / sizeof _nested_build_vehicle_smart_widgets[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this way of testing, but anyway you can use the lengthof() macro that does exactly this, gets the static length of an array.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I initially thought of making a separate menu in a separate file. But much of it would be a copy. And to support both options would be terrible.

@@ -1478,7 +1992,22 @@ struct BuildVehicleWindow : Window {
void SetStringParameters(int widget) const override
{
switch (widget) {
case WID_BV_CAPTION:
case WID_BV_SLOPE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case labels should be indented a level deeper than the switch statement, so the code inside each case is indented two levels deeper than the switch statement.

VehicleFilter& filter = _engine_sort_filter[this->vehicle_type];

switch (widget_for_query)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening brace should be on line above


switch (widget_for_query)
{
case WID_BV_RAIL_SPEED_EDIT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case labels should be indented a level deeper than the switch statement, so the code inside each case is indented two levels deeper than the switch statement.

if (_settings_client.gui.build_vehicle_smart_gui && type == VEH_TRAIN) {
new BuildVehicleWindow(&_build_vehicle_smart_desc, tile, type);
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close brace should be on the same line as the else.

@imcasper imcasper force-pushed the BuildVehicleSmartGUI branch 3 times, most recently from 0ecbf74 to ff5891a Compare July 15, 2019 22:15
@imcasper
Copy link
Author

I'm not sure that everything works as expected when I merge commits. I see the changes that other authors make. In principle, I have led all the code to the desired form, as it seems to me. maybe the problem is that I have a separate branch. Unfortunately, I have never worked according to this scheme. Work with svn for me came down to - commit / update, rarely merge.

@glx22
Copy link
Contributor

glx22 commented Jul 15, 2019

Using a separate branch is the usual way to work with git, but it looks like you squashed way too many commits when rebasing. You included all master commits from f8633fc to a35b43c in your commit.

@imcasper
Copy link
Author

imcasper commented Jul 16, 2019

I hate to squash and change commits anyway. In my opinion this is a violation of the principles of version control systems. Now it seems to me easier to manually collect the changes and apply them to a clean trunk. In any case, I am very grateful for the prompt feedback, which allowed me, as it seems to me to improve my code.
PS By the way, there is probably an opportunity to set the style of code auto-formatting once, and not think about it anymore.

@nielsmh
Copy link
Contributor

nielsmh commented Jul 16, 2019

Multiple, logic commits for a PR are great, as long as the code after each commit is "stable"/"makes sense". For small changes, lots of "sausage making" commits are fine, they can get squashed for merge. For larger changes like this one, I think the consensus is to prefer a pretty commit history in the PR. (I've personally spent far more time than I'd like to clean up my own commit histories and separate changes into more logical commits, where I had originally made commits with unrelated changes lumped together.)

I've tried the feature out now, here's some screenshots.
image image

The second of these already shows one bug: Cars with no speed limit are filtered out when the speed is set anywhere above 0 km/h.

Another bug is when you open two depots and click New Vehicles in both of them. You can't have one of the New Rail Vehicles windows working without filters, and the other working with filters. When you switch one the other also switches. They really ought to be independent.
You can switch between cars/MUs/locos independently in the two open New Rail Vehicles windows, but the filter parameters also sync between the two, so you can't filter for different speed/weight in the two windows.

Here's a screenshot in 1x GUI and font scale:
image
This looks somewhat better to me, but the large paddings in the controls are unlike anything else in the game's GUI, and should probably be changed to look more like it.
I'd also suggest changing the "Use filters" button to be a title bar button instead, either label it "Filter" or maybe just "F". It could also be next to the Sort By dropdown, then the Sort By row should stay at the top of the window when filters are enabled.

The button label "Take the train from the from" makes no sense to me. The tooltip explains it, but the label needs improvement :)
However, I think a better way to handle it would be similar to the Clone tool in the depot. Click the button, then click the train you want to base your search on. That lets you use any train in the depot, or any train visible on screen anywhere.

The way the filter parameters are presented is unusual, look at the Settings window the the game, NewGRFs, and the Cheats window (Ctrl-Alt-C to open) how similar lists of options are laid out.

@andythenorth andythenorth added the stale Stale issues label Nov 2, 2019
@andythenorth
Copy link
Contributor

Thanks for submitting this PR! As there has been no activity on this PR for some, I'm proposing closing it in a few weeks if nothing more has changed, as we try to keep the PR count low. Thanks for contributing!

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while and removed stale Stale issues labels Dec 14, 2020
@TrueBrain
Copy link
Member

Hi @imcasper ; it has been a while :D

I am not sure if you are still interesting in following up on this PR; it seems there are a few comments open, and a few mentions of issues that needs resolving. But as the last poke was over a year ago, I am going to close this PR for now.

This does not mean we are not interested in this work; I have not looked into this myself, but given by the response of other devs, there clearly is interest. If you feel up for it to pick up the last bits and pieces, please reopen the PR and we can help you out.

And if you are just stuck with something or needs further help, etc, just reach out!

Tnx!

@TrueBrain TrueBrain closed this Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants