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
Conversation
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) |
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. |
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.
Pull request name changed.
It's not about the branch name, it's about your commit messages. |
You'll have to use Additionally: Don't modify the Visual Studio project files directly, you should modify the You also need to run the |
src/core/std_type.hpp
Outdated
#include <map> | ||
|
||
template <class Key, class Value> | ||
using Map = std::map<Key, Value>; |
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.
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
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.
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.
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.
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.
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). |
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. |
I understood. Thank. When everything is set up, then I will release the version. |
3e4e8a1
to
a2d74f0
Compare
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.
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
)
src/core/std_type.hpp
Outdated
#include <map> | ||
|
||
template <class Key, class Value> | ||
using Map = std::map<Key, Value>; |
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.
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: |
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.
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()); |
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.
Always use this->
for member functions and variables. Same applies to more functions below.
return GetCustomWeight(this->cargo.StoredCount()); | |
return this->GetCustomWeight(this->cargo.StoredCount()); |
src/lang/english.txt
Outdated
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 |
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.
Spelling: "preferred" (same mistake in several more lines)
src/lang/english.txt
Outdated
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... |
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.
OTTD generally does not use ellipses to indicate whether a button/menu will open a new window. Just leave it out.
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.
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.
src/build_vehicle_gui.cpp
Outdated
} | ||
|
||
bool IsSmartMenu() { | ||
return (window_desc->nwid_length == sizeof _nested_build_vehicle_smart_widgets / sizeof _nested_build_vehicle_smart_widgets[0]); |
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.
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.
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.
I completely agree
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.
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.
src/build_vehicle_gui.cpp
Outdated
@@ -1478,7 +1992,22 @@ struct BuildVehicleWindow : Window { | |||
void SetStringParameters(int widget) const override | |||
{ | |||
switch (widget) { | |||
case WID_BV_CAPTION: | |||
case WID_BV_SLOPE: |
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.
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.
src/build_vehicle_gui.cpp
Outdated
VehicleFilter& filter = _engine_sort_filter[this->vehicle_type]; | ||
|
||
switch (widget_for_query) | ||
{ |
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.
Opening brace should be on line above
|
||
switch (widget_for_query) | ||
{ | ||
case WID_BV_RAIL_SPEED_EDIT: |
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.
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.
src/build_vehicle_gui.cpp
Outdated
if (_settings_client.gui.build_vehicle_smart_gui && type == VEH_TRAIN) { | ||
new BuildVehicleWindow(&_build_vehicle_smart_desc, tile, type); | ||
} | ||
else { |
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.
Close brace should be on the same line as the else
.
0ecbf74
to
ff5891a
Compare
ff5891a
to
775735b
Compare
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. |
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. |
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! |
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! |
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.