Tutorial - code review

[Andarix]

https://github.com/simutrans/tutorial_multipak/commit/b338cc55bdfe861957a99e6b3e35a6e850b9c89b

reupload

I had actually corrected it and re-uploaded it. Apparently, however, the file wasn't updated in the archive.

[Yona-TYT]

Excellent work! The tool and menu icons look great; implementing this was a good idea.  
 

[prissi]

Some testing (in English):
1) before building depot: When building a road (by pressing 's' in pak64), I cannot proceed as I cannot build anything. Only by selecting from the toolbar I can proceed. (Contrary to the text, any road seems to work, even the 40 km/h gavel road.)
2) The line step does not mention (in English) that one can copy the convoi. Also it is unclear in which window the new line is to be created.
3) Heap buffer overflow (with address sanitizer) when selecting the last stop in the schedule for the connenction. Error originat from a call from is too allowed 4108 with param = "D\td\x1\x1\x4ｾｾ@Kr\x5\b\x5ｾｾ\x2\x3" I guess so output is generated from this function which is forbidden. Same if jumoping to this step and click on the bus and then on its schedule.

[Yona-TYT]

2) The line step does not mention (in English) that one can copy the convoi. Also it is unclear in which window the new line is to be created.

For simple vehicles like buses (without trailers, wagons, etc.), I didn't see it as a priority to suggest the copy button at this stage of the tutorial. I thought it was more convenient to use it in Chapter 3, where you have to assemble a passenger train, where this action considerably increases its complexity and using the copy button becomes more indispensable.

[Andarix]

Some testing (in English):
1) before building depot: When building a road (by pressing 's' in pak64), I cannot proceed as I cannot build anything. Only by selecting from the toolbar I can proceed. (Contrary to the text, any road seems to work, even the 40 km/h gavel road.)
...

Any road has always worked because there's no test for it. The same applies to railway tracks.

If I remember correctly, I already suggested years ago that such information be omitted from the text. That's why there are no speed specifications in the German text.

Include speeds in the text because the tutorial would have to be updated every time the package set was changed. Especially since the text no longer applies to a single package set, it should be as independent of the package set as possible.

I haven't changed the English text because my language skills aren't sufficient.

[EDIT]

The tool ID of the button is checked. If the command is issued via keyboard, the tool ID will differ.

...
3) Heap buffer overflow (with address sanitizer) when selecting the last stop in the schedule for the connenction. Error originat from a call from is too allowed 4108 with param = "D\td\x1\x1\x4ｾｾ@Kr\x5\b\x5ｾｾ\x2\x3" I guess so output is generated from this function which is forbidden. Same if jumoping to this step and click on the bus and then on its schedule.

Could you please post a screenshot of the error message?

The text is unreadable here.

[Andarix]

...
[EDIT]

The tool ID of the button is checked. If the command is issued via keyboard, the tool ID will differ.
...

The idea is correct, but the value is wrong.

The `tool_id` is correct. Only the object name is passed differently.

The name is passed via the button.
The value 1 is passed via the key "s".




Docu function
is_work_allowed_here()

The description doesn't seem to be entirely correct either. There, "name" is defined as "coord3d".

I think this s a simutrans bug, not a tutprial bug.

[Yona-TYT]

The idea is correct, but the value is wrong.

The `tool_id` is correct. Only the object name is passed differently.

The name is passed via the button.
The value 1 is passed via the key "s".

There is certainly a strange discrepancy here with the id_name, even though the road is exactly the same.

This bug on the bridge leaves the tutorial stuck.

[prissi]

The bridge bug has been fixed.

The heap overflow happened when checking for the schedule tool in _ia_allowed_tool, the string is the corrdinates for the schedule entry. There is not script message, Simutrans itself has an internal memory error during allocation. The normal executable will write garbage at some memory location but does not crash. Anyway, in today's version I could no longer reproduce it.

Now another text error in airport step D: "Connect the city of Pollingville with airport {sch2} with four busses"

[Andarix]

...
Now another text error in airport step D: "Connect the city of Pollingville with airport {sch2} with four busses"

fixed

[Yona-TYT]

The idea is correct, but the value is wrong.

The `tool_id` is correct. Only the object name is passed differently.

The name is passed via the button.
The value 1 is passed via the key "s".

Wait a minute, is the value 1 the value of the system type? If this is true, then it's easy to solve this on the script side.

[Andarix]

Wait a minute, is the value 1 the value of the system type? If this is true, then it's easy to solve this on the script side.

I think is the waytype. Track (key t) is 2.

It's certainly easy to intercept in the tutorial script.

But according to the documentation, the object name is the parameter to be passed, not the waytype.

Then there are also `e` and `l` currently available in pak64.


I'd be more interested in why the first train breaks down in Chapter 3 when I change Chapter 2.

[Yona-TYT]

I'd be more interested in why the first train breaks down in Chapter 3 when I change Chapter 2.

Okay, I'm going to review this now.

Edit.
The first train in CH3 works fine here, maybe it's only failing in pak64.german?.

[Andarix]

Okay, I'm going to review this now.

Edit.
The first train in CH3 works fine here, maybe it's only failing in pak64.german?.

The current code certainly works. However, since I know the changes will cause errors, I won't be posting such code to the repository.

see this post

[Yona-TYT]

I'm using the latest night of simutrans, the latest version of pak64.german (the link in the install guide), and the latest changes in the tutorial, but it says the savegame is incompatible.

[Andarix]

Changed set_data.nut?

The repository is set to pak64. Consequently, the wrong sve will be loaded if the settings are not changed.

[Yona-TYT]

Yeah, This is how it is at the moment.

[Andarix]

The only thing is that the sve file tutorial64g.sve is still commented out.

[Andarix]

The current code certainly works. However, since I know the changes will cause errors, I won't be posting such code to the repository.

see this post

Oddly enough, these changes have no negative effects from Chapter 3 onwards.

Code Select Expand

class/class_chapter_02.nut | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/class/class_chapter_02.nut b/class/class_chapter_02.nut
index 821d2e1..f63334d 100644
--- a/class/class_chapter_02.nut
+++ b/class/class_chapter_02.nut
@@ -106,12 +106,12 @@ class tutorial.chapter_02 extends basic_chapter
     if(this.step == 4){
       local c_dep = this.my_tile(city1_road_depot)
       local c_list = city1_halt_1
-      start_sch_tmpsw(pl,c_dep, c_list)
+      start_sch_tmpsw(pl, c_dep, c_list)
     }
     else if(this.step == 6){
       local c_dep = this.my_tile(city1_road_depot)
       local c_list = city1_halt_2
-      start_sch_tmpsw(pl,c_dep, c_list)
+      start_sch_tmpsw(pl, c_dep, c_list)
     }
     else if(this.step == 7){
       local c_dep = this.my_tile(city1_road_depot)
@@ -153,8 +153,9 @@ class tutorial.chapter_02 extends basic_chapter
         break
       case 4:
         //local c = coord(city1_halt_1[0].x, city1_halt_1[0].y)
-        local tile = my_tile(city1_halt_1[0])
-        text.stnam = "1) "+tile.get_halt().get_name()+" ("+city1_halt_1[0].tostring()+")"
+        local halt_to_waiting = get_waiting_halt(1)
+        local tile = my_tile(city1_halt_1[halt_to_waiting])
+        text.stnam = (halt_to_waiting+1) + ") "+tile.get_halt().get_name()+" ("+city1_halt_1[halt_to_waiting].tostring()+")"
 
         text.list = create_schedule_list(city1_halt_1)
         text.nr = city1_halt_1.len()
@@ -180,10 +181,10 @@ class tutorial.chapter_02 extends basic_chapter
         }
         text.list = create_schedule_list(city1_halt_2)
 
-        local tile = my_tile(city1_halt_2[0])
-        text.stnam = ""+tile.get_halt().get_name()+" ("+city1_halt_2[0].tostring()+")"
+        local tile = my_tile(city1_halt_2[get_waiting_halt(2)])
+        text.stnam = ""+tile.get_halt().get_name()+" ("+city1_halt_2[get_waiting_halt(2)].tostring()+")"
 
-        local halt = my_tile(city1_halt_2[0]).get_halt()
+        local halt = my_tile(city1_halt_2[get_waiting_halt(2)]).get_halt()
         text.line = get_line_name(halt)
 
         text.cir = cov_nr
@@ -882,7 +883,7 @@ class tutorial.chapter_02 extends basic_chapter
     local nr = schedule.entries.len()
     switch (this.step) {
       case 4:
-        local selc = 0
+        local selc = get_waiting_halt(1)
         local load = veh1_load
         local time = veh1_wait
         local c_list = city1_halt_1
@@ -895,7 +896,7 @@ class tutorial.chapter_02 extends basic_chapter
         return result
       break
       case 6:
-        local selc = 0
+        local selc = get_waiting_halt(2)
         local load = veh1_load
         local time = veh1_wait
         local c_list = city1_halt_2
@@ -910,7 +911,7 @@ class tutorial.chapter_02 extends basic_chapter
         local load = veh1_load
         local time = veh1_wait
         local c_list = city2_halt_1
-        local selc = c_list.len()-1
+        local selc = get_waiting_halt(3)
         result = compare_schedule(result, pl, schedule, selc, load, time, c_list, true)
         if(result == null){
           local line_name = line3_name //"Test 3"
@@ -939,7 +940,7 @@ class tutorial.chapter_02 extends basic_chapter
             reset_tmpsw()
             return bus_result_message(result, translate(name), veh, cov)
           }
-          local selc = 0
+          local selc = get_waiting_halt(1)
           local load = veh1_load
           local time = veh1_wait
           local c_list = city1_halt_1
@@ -958,13 +959,13 @@ class tutorial.chapter_02 extends basic_chapter
           local good_list = [good_desc_x (good_alias.passa).get_catg_index()]    //Passengers
           local name = veh1_obj
           local st_tile = 1
-          result = is_convoy_correct(depot,cov,veh,good_list,name, st_tile)
+          result = is_convoy_correct(depot, cov, veh, good_list, name, st_tile)
           if (result!=null){
             reset_tmpsw()
             return bus_result_message(result, translate(name), veh, cov)
           }
 
-          local selc = 0
+          local selc = get_waiting_halt(2)
           local load = veh1_load
           local time = veh1_wait
           local c_list = city1_halt_2
@@ -1072,7 +1073,7 @@ class tutorial.chapter_02 extends basic_chapter
           local wait = veh1_wait
           local sch_siz = c_list.len()
           for(local j=0;j<sch_siz;j++){
-            if (j==0)
+            if (j==get_waiting_halt(1))
               sched.entries.append(schedule_entry_x(my_tile(c_list[j]), load, wait))
             else
               sched.entries.append(schedule_entry_x(my_tile(c_list[j]), 0, 0))
@@ -1123,7 +1124,7 @@ class tutorial.chapter_02 extends basic_chapter
           local time = veh1_wait
           local sched = schedule_x(gl_wt, [])
           for(local i=0;i<sch_siz;i++){
-            if (i==0)
+            if (i==get_waiting_halt(2))
               sched.entries.append(schedule_entry_x(my_tile(c_list[i]), load, time))
             else
               sched.entries.append(schedule_entry_x(my_tile(c_list[i]), 0, 0))
@@ -1175,7 +1176,7 @@ class tutorial.chapter_02 extends basic_chapter
           local c_list = city2_halt_1
           local sch_siz = c_list.len()
           for(local j=0;j<sch_siz;j++){
-            if (j==sch_siz-1)
+            if (j==get_waiting_halt(3))
               sched.entries.append(schedule_entry_x(my_tile(c_list[j]), load, wait))
             else
               sched.entries.append(schedule_entry_x(my_tile(c_list[j]), 0, 0))


[Yona-TYT]

It's strange.

The current code from the repo (tutorial_multipak-code_review.zip) works. If I modify chapter 2 (class_chapter_02.nut), then chapter 2 works correctly. But in chapter 3, the first train (step E) is broken.

As a player, I can't start it.
The train isn't even created using automatic steps.

This relates to the logic used to determine the number of vehicles and whether their IDs are valid. If you look at the garage, there's a vehicle that was probably created too many and never started.

I need to investigate this further. Tell me, what exactly did you modify in CH2?.

[Andarix]

This relates to the logic used to determine the number of vehicles and whether their IDs are valid. If you look at the garage, there's a vehicle that was probably created too many and never started.
...

Then, apparently, the wrong depot is queried in Chapter 3.

...
I need to investigate this further. Tell me, what exactly did you modify in CH2?.
...

In the post before her post.

[Yona-TYT]

Then, apparently, the wrong depot is queried in Chapter 3.

In the post before her post.

I realized that using the automated script, the last vehicle in chapter 2 is created correctly, but for some reason it doesn't start from the depot, so the problem is in chapter 2, not chapter 3.

Edit.
Confirmed, creating and starting the vehicle manually works fine, the problem is in the automated script logic for the vehicle in chapter 2, step G.

[Yona-TYT]

I realized that using the automated script, the last vehicle in chapter 2 is created correctly, but for some reason it doesn't start from the depot, so the problem is in chapter 2, not chapter 3.

Edit.
Confirmed, creating and starting the vehicle manually works fine, the problem is in the automated script logic for the vehicle in chapter 2, step G.

That's why the script advances when starting vehicles are done using conditionals like this

Code Select Expand

if (cov_valid && current_cov == ch2_cov_lim1.b){ pot2=1 }
, which prevents the script from advancing when a vehicle fails to start.

Edit.
I have an idea: to do a "follow vehicle" using a command. Is this implemented?.

Edit.

Code Select Expand

          for(local j=0;j<sch_siz;j++){
            if (j==get_waiting_halt(3))
              sched.entries.append(schedule_entry_x(my_tile(c_list[j]), load, wait))
            else
              sched.entries.append(schedule_entry_x(my_tile(c_list[j]), 0, 0))
          }
I can trace the problem so far, the construction of the schedule is wrong, "get_waiting_halt(3)" is 0, but the value of that condition is required to be the total size of the list (len - 1), since the stop to which the values ��are applied is the last stop of the list.


Edit.

Applying the following patch "https://forum.simutrans.com/index.php/topic,23781.msg211954.html#msg211954", everything would look like this: class_chapter_02.nut