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

TBTR 2.0 (Template-based Train replacement) #7145

Closed
wants to merge 0 commits into from

Conversation

@flitzpiepe
Copy link

flitzpiepe commented Jan 30, 2019

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

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Jan 30, 2019

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!

Copy link
Contributor

nielsmh left a comment

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

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

This comment has been minimized.

Copy link
@nielsmh

nielsmh Jan 30, 2019

Contributor

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.

This comment has been minimized.

Copy link
@LordAro

LordAro Jan 30, 2019

Member

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

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

This comment has been minimized.

Copy link
@nielsmh

nielsmh Jan 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Jan 30, 2019

Member

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

This comment has been minimized.

Copy link
@flitzpiepe

flitzpiepe Jan 30, 2019

Author

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.

This comment has been minimized.

Copy link
@Eddi-z

Eddi-z Jan 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@flitzpiepe

flitzpiepe Jan 30, 2019

Author

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

src/group.h Outdated
@@ -18,6 +18,9 @@
#include "vehicle_type.h"
#include "engine_type.h"

typedef int16 TemplateID;

This comment has been minimized.

Copy link
@nielsmh

nielsmh Jan 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@flitzpiepe

flitzpiepe Jan 30, 2019

Author

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

#include "vehicle_base.h"
#include "vehicle_func.h"

typedef int16 TemplateID;

This comment has been minimized.

Copy link
@nielsmh

nielsmh Jan 30, 2019

Contributor

This is repeated from group.h

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

This comment has been minimized.

Copy link
@LordAro

LordAro Jan 30, 2019

Member

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

@flitzpiepe

This comment has been minimized.

Copy link
Author

flitzpiepe commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Author

flitzpiepe commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

planetmaker commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Author

flitzpiepe commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jan 31, 2019

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

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 1, 2019

[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

This comment has been minimized.

Copy link
Member

PeterN commented Feb 1, 2019

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.

tadinghattan transport 17th oct 2050

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 1, 2019

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.
@@ -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),

This comment has been minimized.

Copy link
@glx22

glx22 Feb 1, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@glx22

glx22 Feb 1, 2019

Contributor

why not "keep remainders" ?


#include "saveload.h"

const SaveLoad* GTD() {

This comment has been minimized.

Copy link
@glx22

glx22 Feb 1, 2019

Contributor

It's not a macro, use a descriptive name

@flitzpiepe flitzpiepe closed this Feb 2, 2019
@flitzpiepe flitzpiepe force-pushed the flitzpiepe:master branch from a05fe89 to 8334a57 Feb 2, 2019
@flitzpiepe

This comment has been minimized.

Copy link
Author

flitzpiepe commented Feb 2, 2019

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

This comment has been minimized.

Copy link
Contributor

planetmaker commented Feb 2, 2019

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

@flitzpiepe

This comment has been minimized.

Copy link
Author

flitzpiepe commented Feb 2, 2019

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

This comment has been minimized.

Copy link
Contributor

planetmaker commented Feb 2, 2019

I see. Funky github :)

@flitzpiepe

This comment has been minimized.

Copy link
Author

flitzpiepe commented Feb 3, 2019

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

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Feb 3, 2019

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

This comment has been minimized.

Copy link
Author

flitzpiepe commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Author

flitzpiepe commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 4, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.