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

Open
wants to merge 3 commits into
base: master
from
Open

Conversation

@imcasper
Copy link

imcasper commented Jul 13, 2019

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 left a comment

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

This comment has been minimized.

@LordAro

LordAro Jul 14, 2019 Member

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

This comment has been minimized.

@imcasper

imcasper Jul 15, 2019 Author

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.

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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

imcasper commented Jul 14, 2019

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

imcasper commented Jul 14, 2019

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

@imcasper imcasper force-pushed the imcasper:BuildVehicleSmartGUI branch from 3e4e8a1 to a2d74f0 Jul 15, 2019
Copy link
Contributor

nielsmh left a comment

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

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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:

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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());

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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

This comment has been minimized.

@imcasper

imcasper Jul 15, 2019 Author

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]);

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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.

This comment has been minimized.

@imcasper

imcasper Jul 15, 2019 Author

I completely agree

This comment has been minimized.

@imcasper

imcasper Jul 15, 2019 Author

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:

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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)
{

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

Opening brace should be on line above


switch (widget_for_query)
{
case WID_BV_RAIL_SPEED_EDIT:

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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 {

This comment has been minimized.

@nielsmh

nielsmh Jul 15, 2019 Contributor

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

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

imcasper commented Jul 15, 2019

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 label Nov 2, 2019
@andythenorth
Copy link
Contributor

andythenorth commented Nov 2, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.