Skip screen for "Preview Engine" if Vehicle type is not used

[Eagle_rainbow]

Hi everyone,

Playing OTTD at my favorite server regularly, I noticed that the players get the engine preview screen also for those vehicles types which they cannot build at all, i.e. by game settings such as

Code: Select all

max_aircraft=0

or similar. Assuming that the server's settings do not change, no homo economicus-styled player would click on the "accept preview" button, as he couldn't build any such engine and thus would be blocked for further (then buildable) engine previews for the next few years.
When looking at one of those screens, I started asking myself why it comes up at all. Searching here in the forum I did not succeed to find anything helpful. Having a second look at the coding, I noticed that there simply is no check on that situation as well.

Is there a certain reason for this behaviour which I am missing out? What would be the benefit to click on "accept" in that case?

If there is none, I would like to suggest suppressing those screens. When searching through the coding I think I already spotted an easy way to do so...

[Yexo]

Is there a certain reason for this behaviour which I am missing out?

Nobody implemented a check for this.

What would be the benefit to click on "accept" in that case?

None.

If there is none, I would like to suggest suppressing those screens.

That is a good suggestion.

When searching through the coding I think I already spotted an easy way to do so...

Please write a patch and post it here or on bugs.openttd.org for review.

[XeryusTC]

This is actually a really good suggestion, it is weird that no one has suggested this before. I think it would be especially awesome if the same thing would also happen for the new vehicle news item in general, it would clean up the bottom of my screen quite a lot

[Eagle_rainbow]

Please write a patch and post it here or on bugs.openttd.org for review.

Please find attached a first draft for such a patch. Feedback is appreciated - hopefully, I followed the coding style guide better now

I think it would be especially awesome if the same thing would also happen for the new vehicle news item in general[...]

The patch also includes this request: You won't get any news item anymore (even not in history) for engines which you cannot build (due to settings).

I have tested the patch already both in single-user mode and in LAN multiplayer mode. I couldn't find anything of concern. It even can handle the situation, if the advanced settings are changed during gameplay: if a preview screen was suppressed before and the engine is not available publicly yet, but building the engine is allowed now again, the preview screen will be shown on the next switch of the month (normal procedure on the sequence of companies is resumed).
However, it should be noted that I have my doubts when having a server that is not on the same revision than the client (or vice versa) with this patch. Yet, this matter doesn't seem to be new, as the revision is used during protocol handshaking. So, I don't think that this is posing an issue.

[Yexo]

However, it should be noted that I have my doubts when having a server that is not on the same revision than the client (or vice versa) with this patch..

In no circumstances is a client allowed to have a different version than the server, so this is a non-issue.

Some remarks about your patch (haven't tested it yet, just reviewed the code):

Code: Select all

+ * @param type the vehicle type which shall be checked
+ * @return False, if the settings currently prohibit building such a vehicle;
+ * true, if it can be built.

Instead I'd write the following: (Capital T and dot in first sentence have to change, second sentence can be debated).

Code: Select all

+ * @param type The vehicle type which shall be checked.
+ * @return Whether or not a vehicle type can be build with regards to vehicle.max_* settings.

Code: Select all

if (_settings_game.vehicle.max_trains == 0) return false;
break;

Could be simplified to

Code: Select all

return _settings_game.vehicle.max_trains > 0;

Code: Select all

AI::BroadcastNewEvent(new ScriptEventEngineAvailable(index));

I think this line should be inside the new if-block too. For AIs it would make sense to additionally check if AIs are allowed to build the vehicle type.

I'm wondering if there is no existing function that does the same as IsVehicleTypeBuildable that can be reused. Overall I really like the simplicity of the change. Nice work on adapting to the coding style so fast, as you can see above I'm really nitpicking now on some last details, but changes like these won't hold back a patch for inclusion (as in, I'd fix it myself before committing since it's just very small changes). It'd still be appreciated if you could change them and post a new patch

[Eagle_rainbow]

Nice work on adapting to the coding style so fast, as you can see above I'm really nitpicking now on some last details [...]

Thanks, I am doing my very best
And thank you that you are taking so much time to go over my coding over and over again. I hope it will pay off...

It'd still be appreciated if you could change them and post a new patch

Sure, here we go...

Code: Select all

+ * @param type the vehicle type which shall be checked

Instead I'd write the following: (Capital T and dot in first sentence have to change[...]).

If I may be nitpicking here as well I don't get the hang of it when you want to have it with capital letters and dot at the end. Currently, I am doing it "gut-feeling-wise" plus the insight "in dubio pro capitalization". This phrase isn't a full sentence anyway, because it is missing the verb ("checked" is in the subclause only). But anyway, we can also change this one here I don't mind...

Could be simplified to

Code: Select all

return _settings_game.vehicle.max_trains > 0;

Done. Other structurally equal cases adjusted as well.

Code: Select all

AI::BroadcastNewEvent(new ScriptEventEngineAvailable(index));

I think this line should be inside the new if-block too. For AIs it would make sense to additionally check if AIs are allowed to build the vehicle type.

When implementing I had a thought on that one as well. As I don't know the AI's interface at all, yet, I decided to remain cautious here: Putting it in the if block could look like a contract change in an incompatible manner for the AI developer where he/she wasn't notified timely. Thus, I was hesitant to do so, as I couldn't estimate on the impact of my doing. But as you don't seem to be too much worried about that, we'll go for it...

I'm wondering if there is no existing function that does the same as IsVehicleTypeBuildable that can be reused.

Well, thought so as well. I found IsEngineBuildable, but that one is dealing with the question whether the engine is available to a company at all and thus I couldn't use it (BTW in my eyes that function should better be named "MayCompanyBuildEngine" or "IsEngineBuildableByCompany" or similar). Other functions need to check _settings_game.vehicle.max_trains in some sense and a repository-global string search on this term didn't reveil anything suspicious as well.
Additionally, I am also not too convinced about the signature of IsVehicleTypeBuildable myself: Its generality and the coding complexity suggest to make it an inline function. But this then should go to some header file, whilst there is none that is really infliciting on me. Alternatively, I also thought about it making an inline method of class Engine. On the other hand, if we do so, consistency would suggest to also move the function "IsEngineBuildable" to a method of that class. Though I am willing to also head out for all consumers of this function, I now fear that this topic begins to trigger a fundamental debate - and moreover, the simplicity of my patch, which you liked so much (and was also compelling to me), will be gone for a toss...

[Yexo]

Code: Select all

+ * @param type the vehicle type which shall be checked

Instead I'd write the following: (Capital T and dot in first sentence have to change[...]).

If I may be nitpicking here as well I don't get the hang of it when you want to have it with capital letters and dot at the end. Currently, I am doing it "gut-feeling-wise" plus the insight "in dubio pro capitalization". This phrase isn't a full sentence anyway, because it is missing the verb ("checked" is in the subclause only). But anyway, we can also change this one here I don't mind...

You're right about this, I'm not sure whether there are any written rules in the wiki or somewhere else about when to use caps. Personally I find that in "@param type the vehicle" the difference between the parameter name "type" and the description "the vehicle..." is not very clear, and the tiny change of capitalizing the first word of the description helps with that. Existing code is not very consistent with regards to this.
Others might have different opinions about this or other reasons to capitalize / not capitalize in those cases.

I'm wondering if there is no existing function that does the same as IsVehicleTypeBuildable that can be reused.

Well, thought so as well. I found IsEngineBuildable, but that one is dealing with the question whether the engine is available to a company at all and thus I couldn't use it (BTW in my eyes that function should better be named "MayCompanyBuildEngine" or "IsEngineBuildableByCompany" or similar).

IsEngineBuildable is not useable as you found out, is has a different function.

Additionally, I am also not too convinced about the signature of IsVehicleTypeBuildable myself: Its generality and the coding complexity suggest to make it an inline function. But this then should go to some header file, whilst there is none that is really infliciting on me.

Marking the function as "inline" and placing it in a header file has no direct relation.

Code: Select all

static inline bool IsVehicleTypeBuildable(VehicleType type)

would be perfectly fine in this case. Even without the inline I'd guess that all compilers would inline the function anyway, so it doesn't really matter.
Please leave out the const for VehicleType, since it's an enum. For POD types the "const" is superfluous and so far the consensus seems to be to leave it out.

Alternatively, I also thought about it making an inline method of class Engine.

It doesn't belong there, this is not a property of a single engine but of a whole class of engines.

On the other hand, if we do so, consistency would suggest to also move the function "IsEngineBuildable" to a method of that class. Though I am willing to also head out for all consumers of this function, I now fear that this topic begins to trigger a fundamental debate - and moreover, the simplicity of my patch, which you liked so much (and was also compelling to me), will be gone for a toss...

Making IsEngineBuildable a member of class Engine could be a good idea, but it doesn't have any relation to the current patch. Even if decided we'd want that, it should be a different patch and as such not influence the complexity of this one.

Code: Select all

// all other types cannot be built anyway

In this case there is no doubt, "All" and a missing dot since this is a full sentence.

[Eagle_rainbow]

Code: Select all

static inline bool IsVehicleTypeBuildable(VehicleType type)

would be perfectly fine in this case.

Then we have found the signature of that function... changed in the new version.

Code: Select all

// all other types cannot be built anyway

In this case there is no doubt, "All" and a missing dot since this is a full sentence.

Touché

Here's the new version.

[Michi_cc]

Others might have different opinions about this or other reasons to capitalize / not capitalize in those cases.

Personally I prefer capitalization and proper punctuation as well, but if I amend existing code I'll try to match the existing style.

For POD types the "const" is superfluous and so far the consensus seems to be to leave it out.

We do indeed normally leave the const out, but it is in fact not completely superfluous. A const will prevent accidentally reassigning the variable in the function body. OTOH, there are many places where we are reassigning deliberately to save extra variables, so the code would end up appearing inconsistent at first glance. And hey, this is C++ and not Java

-- Michael Lutz

[Alberth]

Some small remarks:

Code: Select all

static inline bool IsVehicleTypeBuildable(VehicleType type)

I would leave the 'inline' out, but is mostly a matter of taste.

"Buildable" however does not really cover the intention imho; a vehicle type could be unbuildable for many reasons (reached settings max limit, too early, too late).
Perhaps "Available" is better?

As for the code inside the switch, the code style indeed says to indent the code 1 tab, like you do. It does however also say to leave an empty line between the cases.
However, in this case, I'd put the 'return' statement directly after the "case X:". I see not much point in spreading the case on two or three lines.

For places in the code that should never be reached, there is a NOT_REACHED() macro which fails hard if the impossible happens.
That's better than silently returning a safe value (and also self-documenting).

Code: Select all

+	/* Suppress "New Vehicle available" news paper entry, if engine cannot be built anyway. */
+	if (IsVehicleTypeBuildable(e->type)) {

Your comment says the opposite of what the code does (the 'if' does not suppress the announcement).

[Yexo]

Code: Select all

+	/* Suppress "New Vehicle available" news paper entry, if engine cannot be built anyway. */
+	if (IsVehicleTypeBuildable(e->type)) {

Your comment says the opposite of what the code does (the 'if' does not suppress the announcement).

Comment and code agree. The only confusion here can arise from the fact that the comment has a double negative, "suppress... if cannot be built" (=show if can be built).

Fully agree with your other comments. The inline is indeed a matter of taste and I don't think I'd have written it myself had I written the patch, but I wouldn't have removed it either if it was already there.

[Eagle_rainbow]

As for the code inside the switch,

Adjusted as you suggested.

"Buildable" however does not really cover the intention imho; [...] Perhaps "Available" is better?

I know what you mean. However, "available" is something different to me... An engine, for example, gets "available" to me, once the preview period is over and its availability is public. What about "permitted" or "allowed"?

[NOT_REACHED() is] better than silently returning a safe value (and also self-documenting).

done, although my thought there was, that my answer on all the other types is not just "a safe value" but really the truth (a player can't build UFOs )

Code: Select all

+	/* Suppress "New Vehicle available" news paper entry, if engine cannot be built anyway. */
+	if (IsVehicleTypeBuildable(e->type)) {

Your comment says the opposite of what the code does (the 'if' does not suppress the announcement).

Comment and code agree. [...]

Apparently, the comment can be mistaken, and that's always bad for a comment. Thus, I have changed it to

Code: Select all

/* Only provide the "New Vehicle available" news paper entry, if engine can be built. */

which I hope is less misleading.

The inline is indeed a matter of taste and I don't think I'd have written it myself had I written the patch, but I wouldn't have removed it either if it was already there.

As there is now the majority against the "inline" statement, I have changed it back again.

Newest version (updated to the current SVN revision)

suppress_preview-and-newspaper-v4-against-r24658.patch

code reviews by Alberth and Yexo incorporated
against trunk r24658 (updated)

(2.66 KiB) Downloaded 120 times

PS: Is there a better way than attaching a new patch's version all the time?

[Yexo]

Committed in r24660. If you're interested in the review process, be sure to read some logs of #openttd.dev. Link can be found in a sticky in the development section.

[Eagle_rainbow]

If you're interested in the review process, be sure to read some logs of #openttd.dev.

Thanks - wasn't aware of those logs yet. The comments there are quite short, so it's hard to read, but I was able to recognize some of the discussions we had already