TBTR 2.0 (Template-based Train replacement) #7145

flitzpiepe · 2019-01-30T17:29:09Z

Hi,

this patch has been available in the tt-forums under:
https://www.tt-forums.net/viewtopic.php?f=33&t=58904

Version 2.0 is a reimplementation which increases usability and significantly cleans up the implementation and makes the patch much more stable.

Eddi-z · 2019-01-30T18:15:13Z

Thank you for your patch! But before we start to review it, a few suggestions:

Please consider grouping commits into more logical units, squashing a few minor edits, and adhere to the commit message style as explained in CONTRIBUTING.md.

Don't be afraid to "git push -f", github can handle that.

Thanks!

nielsmh

In addition to Eddi's general comments, I have a few specifics.

nielsmh · 2019-01-30T17:42:51Z

README_TBTR.md

@@ -0,0 +1,107 @@
+# OpenTTD TBTR


This file needs to go, readme files for individual features are not relevant for the master. Any documentation for the feature should instead be put on the wiki, when the feature is in.

it could go into docs/ though, with a different name

nielsmh · 2019-01-30T17:43:15Z

.gitignore

@@ -50,3 +50,4 @@ src/os/windows/ottdres.rc
 !/config.lib
 !*.in
 *.tmp
+*.swp


What are .swp files and why do they need ignoring?

Remember, that as vim user you can also add these to your global git ignore :)

Those are vim swap files, they are not relevant to the actual code and I wanted them not to show up in the git status. I can remove this from the .gitignore as well if it's an issue.

you can set a global gitignore file like this:
git config --global core.excludesfile '~/.gitignore'
there you can put all editor tempfiles

it doesn't make sense to put them into the repo, because everyone uses a different editor

Thanks for the tip, I actually never thought of doing that.
I will revert this change then.

nielsmh · 2019-01-30T17:45:51Z

src/group.h

@@ -18,6 +18,9 @@
 #include "vehicle_type.h"
 #include "engine_type.h"

+typedef int16 TemplateID;


Why signed? In general object IDs are unsigned in the rest of the code.

True, unsigned makes more sense, this will be changed in the future.

nielsmh · 2019-01-30T18:07:12Z

src/tbtr_template_vehicle.h

+#include "vehicle_base.h"
+#include "vehicle_func.h"
+
+typedef int16 TemplateID;


This is repeated from group.h

LordAro · 2019-01-30T19:44:47Z

source.list

@@ -87,6 +87,9 @@ stringfilter.cpp
 strings.cpp
 story.cpp
 subsidy.cpp
+tbtr_gui.cpp


you'll want to run projects/generate to update the VS project files

flitzpiepe · 2019-01-31T17:11:28Z

Thank you for your patch! But before we start to review it, a few suggestions:

Please consider grouping commits into more logical units, squashing a few minor edits, and adhere to the commit message style as explained in CONTRIBUTING.md.

Don't be afraid to "git push -f", github can handle that.

Thanks!

When reorganizing the existing commit history, is it mandatory that the code compiles and is fully functional at each intermediate commit? Currently this should be the case in ~99% of the commits.

nielsmh · 2019-01-31T17:17:59Z

Yes, each commit should compile and be playable. So you'd generally structure as stages of preparation, change, cleanup, for large additions like this.

Preparation would be refactoring and adding any new framework required, change the actual additions, and cleanup generally ends up just being removing any translation strings no longer needed.

The commit history should as far as possible contain logical changes, rather than read as a developer diary.

flitzpiepe · 2019-01-31T17:25:14Z

That's a lot of changes ;)
I will better just disect the current patch and recommit everything in those logical chunks then.

In principal that sounds to me like I should squash any developed new feature of the patch into a single commit so that it is grouped.
Is there a maximum for the lines-of-code per commit that I should keep an eye on?

planetmaker · 2019-01-31T17:27:46Z

There is no maximum-lines-per-commit.

But there is a soft maximum-lines-per-commit which any person can reasonably review. Which in essence is also one reason for the requirement to break it down into the logical sequence of commits which niels just outlined: a patch series for a big feature structured this way is basically the only way another person can actually review it and follow the code and judge what it does.

Thus make the individual patches reasonably small (not necessarily as small as possible - but squashing is far easier than pulling it apart). Make each address one step on the journey from here to the final feature.

EDIT: consider it the difference between getting 207 unnumbered pages which make the pages of a story vs vs. getting 207 pages which are sequentially numbered, sorted and chapters given nice names

Eddi-z · 2019-01-31T17:34:39Z

As a crude rule of thumb you could start with squashing any commit that contains the word "fix" that doesn't touch anything that existed before this branch was started.

Also, "Merge" commits are probably better avoided by rebasing the appropriate branches.

flitzpiepe · 2019-01-31T17:44:07Z

I was thinking of replacing the current commit history the following list of commits:

definition of class TemplateVehicle
clone TemplateVehicle from Train, delete template
main UI
listing of player's groups + templates
assigning a template to a group
main template replacement procedure
display engine list in main UI
add engines, remove engines from templates

nielsmh · 2019-01-31T17:46:11Z

I'd personally add the logic and commands first, then build the UI on top of that afterwards.

PeterN · 2019-02-01T09:08:33Z

[SRC] Compiling build_vehicle_gui.cpp
In file included from /home/petern/Projects/OpenTTD/src/build_vehicle_gui.cpp:35:
/home/petern/Projects/OpenTTD/src/tbtr_template_vehicle.h:52:1: error: 'TemplateVehicle' defined as a struct here but previously declared as a class [-Werror,-Wmismatched-tags]
struct TemplateVehicle : TemplatePool::PoolItem<&_template_pool>, BaseVehicle {

PeterN · 2019-02-01T09:24:27Z

First impressions on the UI:

The UI does not take account of GUI/Font scaling. Check how this is done in other windows.
We don't use lower buttons as labels anywhere, so this is very jarring.
The UI is pretty unintuitive, and...
Tooltips are wrong. It is using tooltips from the original autoreplace window, but not even on the correct widgets.

PeterN · 2019-02-01T09:48:53Z

A quick look through the code:

Commented out code added?
Incorrectly acting on return value of DoCommandP(). This is asynchronous. You need to respond either to an InvalidateData call, or using a callback if doing something interactive.

glx22 · 2019-02-01T14:49:11Z

src/saveload/group_sl.cpp

@@ -24,6 +24,7 @@ static const SaveLoad _group_desc[] = {
 	     SLE_VAR(Group, vehicle_type,       SLE_UINT8),
 	     SLE_VAR(Group, replace_protection, SLE_BOOL),
 	 SLE_CONDVAR(Group, parent,             SLE_UINT16,                    189, SL_MAX_VERSION),
+	 SLE_VAR(Group, 	template_id, 		SLE_INT16),


Alignment, and needs a savegame bump and SLE_CONDVAR() I think

glx22 · 2019-02-01T14:49:41Z

src/lang/english.txt

+STR_TBTR_SET_REFIT_TIP                                          :{BLACK}If set, the train will use exactly the cargo refit specified by the template. If not every wagon that is to be newly bought or retrieved from the depot, will *attempt* to be refitted as the old one was. Standard refit if this is impossible.
+
+STR_TBTR_CONFIG_USEDEPOT                                        :use depot
+STR_TBTR_CONFIG_KEEPREMAINDERS                                  :keep rem


why not "keep remainders" ?

glx22 · 2019-02-01T14:52:54Z

src/saveload/tbtr_template_vehicle_sl.cpp

+
+#include "saveload.h"
+
+const SaveLoad* GTD() {


It's not a macro, use a descriptive name

flitzpiepe · 2019-02-02T09:40:34Z

Hi,
thanks for all suggestions and sorry for the messy request... :)
I have removed the merge commit from my patch into master. After cleaning everything up a bit I will rebase the topic onto the current master branch and create a new pull request or reopen this one.

planetmaker · 2019-02-02T09:46:17Z

If possible keep this one. It's good to have conversations in one place to avoid repetitions. (why closing?)

flitzpiepe · 2019-02-02T10:08:38Z

I agree about keeping the conversation. The close was done automatically. Before I had not made the pull request for my topic branch, but I had merged the topic into master locally and created the pull request for the master. I removed this merge commit again and the PR was closed by it when I force-pushed it onto my fork.

planetmaker · 2019-02-02T10:19:51Z

I see. Funky github :)

flitzpiepe · 2019-02-03T14:55:51Z

A quick look through the code:

* Commented out code added?

* Incorrectly acting on return value of DoCommandP(). This is asynchronous. You need to respond either to an InvalidateData call, or using a callback if doing something interactive.

I don't understand the second part about the DoCommandP() return value. The definition says that the return value of DoCommandP tells about whether the called command was successful or not. It is used in exactly this way e.g. in industry_gui.cpp:618. I am also using it just to check the result of the called command and to take further actions only if it succeeded.

Eddi-z · 2019-02-03T15:11:13Z

You can call DoCommandP, but the return value won't be meaningful in Network games, as the command must be passed to the server first, which might take some time, and thus the game continues without actually executing the command for a while (this is called "asynchronous", as the result will only come in when the program is already at a completely different place in the code, instead of pausing until the result comes in)

flitzpiepe · 2019-02-04T16:24:11Z

So it means that not even the game client which has issued the DoCommandP call will execute the according command immediately, but will only send it to the server and will later receive an answer back to launch the command?
I had assumed that the client would himself execute the command immediately and would additionally send it to the server for distribution to all other clients.

Eddi-z · 2019-02-04T16:27:30Z

No, because all clients have to execute the command on the exact same tick, the client that sent it must wait for confirmation that the server has distributed it to all clients before executing it. (also, the server might say the command cannot be run for some other reason that the client didn't know about)

flitzpiepe · 2019-02-04T16:48:48Z

Ok thanks, I will put the conditional code afterwards into a callback then.
Btw, what about this instance I found in the industry_gui.cpp:618 ? There the return value of the DoCommandP is used. Also in town_gui.cpp:1126.

glx22 · 2019-02-04T17:20:36Z

For town_gui.cpp I think it happens only in editor. And I can't find your industry_gui.cpp example. Maybe you meant line 670, in this case it just resets the cursor, nothing vital.

nielsmh reviewed Jan 30, 2019

View reviewed changes

nielsmh added enhancement wip savegame upgrade labels Jan 30, 2019

LordAro reviewed Jan 30, 2019

View reviewed changes

glx22 reviewed Feb 1, 2019

View reviewed changes

flitzpiepe closed this Feb 2, 2019

flitzpiepe force-pushed the master branch from a05fe89 to 8334a57 Compare February 2, 2019 09:34

TBTR 2.0 (Template-based Train replacement) #7145

TBTR 2.0 (Template-based Train replacement) #7145

Conversation

flitzpiepe commented Jan 30, 2019

Eddi-z commented Jan 30, 2019 • edited Loading

nielsmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flitzpiepe commented Jan 31, 2019

nielsmh commented Jan 31, 2019

flitzpiepe commented Jan 31, 2019

planetmaker commented Jan 31, 2019 • edited Loading

Eddi-z commented Jan 31, 2019

flitzpiepe commented Jan 31, 2019

nielsmh commented Jan 31, 2019

PeterN commented Feb 1, 2019 • edited Loading

PeterN commented Feb 1, 2019

PeterN commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flitzpiepe commented Feb 2, 2019

planetmaker commented Feb 2, 2019

flitzpiepe commented Feb 2, 2019

planetmaker commented Feb 2, 2019

flitzpiepe commented Feb 3, 2019

Eddi-z commented Feb 3, 2019

flitzpiepe commented Feb 4, 2019

Eddi-z commented Feb 4, 2019

flitzpiepe commented Feb 4, 2019

glx22 commented Feb 4, 2019 • edited Loading

Eddi-z commented Jan 30, 2019 •

edited

Loading

planetmaker commented Jan 31, 2019 •

edited

Loading

PeterN commented Feb 1, 2019 •

edited

Loading

glx22 commented Feb 4, 2019 •

edited

Loading