The International Simutrans Forum

 

Author Topic: Changes to the smoke-parameter for buildings (and ideas around that)  (Read 7119 times)

0 Members and 1 Guest are viewing this topic.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9729
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #35 on: February 23, 2020, 08:00:32 AM »
The only thing I can say for sure, is that version 99 and older had a much worse movement code. Each tile was exactly 16 steps, which looked very ugly in larger pak sizes (even pak128).

But this is not relevant for your case. You need to extend makeobj and the descriptor (so there is more than one offset save in memory).
Instead
Code: [Select]
SmokeTile=1,0
SmokeOffset=0,-98
SmokeSpeed=12
you need to look for
Code: [Select]
SmokeTile[0]=1,0
SmokeOffset[0]=0,-98
SmokeSpeed[0]=12
SmokeTile[1]=0,1
...
and convert the old entires (in makeobj and in Simutrasn when loading old paks, if there is a rotation). Then, when rotating, you just take the new offset from the extended descriptor.

Offline Vladki cz

  • Devotee
  • *
  • Posts: 2920
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #36 on: February 23, 2020, 11:13:12 AM »
Thanks prissi, but that is not what I asked for...

Before implementing multiple smokes per industry (or any building), I want to get the single smoke right. And for that I need to understand the what is going on with the values. Why they are at several places multiplied by OBJECT_OFFSET_STEPS just to be immediately divided by 16, which returns essentially the same value?

The only place I would expect this to happen is when reading the pak file, to scale the offset from 16 steps/tile used by pakset author to the current value that was compiled. But then all calculations should be done with whatever step size is defined. So I would expect it in load/save/reader methods, which is the case of simobj.cc, simroadtraffic.cc, but not in simfab.cc

The code in simvehicle.cc
Code: [Select]
(dx * steps * OBJECT_OFFSET_STEPS) >> 8 seems to be conversion between VEHICLE_STEPS_PER_TILE (256) and OBJECT_OFFSET_STEPS (16), so it should be:
Code: [Select]
(dx * steps * OBJECT_OFFSET_STEPS) >> VEHICLE_STEPS_PER_TILE_SHIFT

Also in wolke.cc the shift ">> 12" seems to be just some arbitrary value set to "look good" in comparison with max. insta_zeit being another magic constant 2500.

Offline Vladki cz

  • Devotee
  • *
  • Posts: 2920
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #37 on: February 23, 2020, 03:10:29 PM »
It just freezes, and it starts out at a random frame to begin with.

Sorry for double post. But wouldn't it be easier then to make the animations to start always at the first frame, and return to first frame if stopped? Then you could do everything just using animations. That should not be that complicated as fixing the smokes

Offline Vladki cz

  • Devotee
  • *
  • Posts: 2920
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #38 on: February 26, 2020, 09:06:57 PM »
I found another weird thing that breaks the smokes behavior on rotation.
Why is there the: if(xoff!=0)? It breaks rotations of stuff that has xoff zero but yoff not zero...

Simobj.cc:
Code: [Select]
void obj_t::rotate90()
{
        // most basic: rotate coordinate
        pos.rotate90( welt->get_size().y-1 );
        if(xoff!=0) {
                sint8 new_dx = -2*yoff;
                yoff = xoff/2;
                xoff = new_dx;
        }
}

Also what sort of disaster could happen if the xoff and yoff would be changed from sint8 to sint16 ?   y_offset 128 is not enought for some high chimneys in pak128.britain :-(

Offline Freahk

  • *
  • Posts: 592
  • Languages: DE, EN
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #39 on: February 26, 2020, 10:33:18 PM »
Also what sort of disaster could happen if the xoff and yoff would be changed from sint8 to sint16
In the rotation itself, there should not be any disasters (I would not expect yoff >= 2^15 to happen)
simobj_t::rdwr: We do some calculations here that will overflow if xoff>=2^12 Further, we will need to ensure we save and load the 16 bit in newer versions, whilst loading 8 bits for older versions.
Sidenote: We actually calculate 16*xoff/16 there, so this seems strange anyway.

In further places spread around the code, however, there will happen some disasters. Most of them simply being truncating the sint16 to sint8, some further overflows might also happen

In any case, "Simply" changing this to 16 bits will cause many truncation disasters spread around the code. Some further disasters unknown type might also happen.
The following functions need some adjustments or considerations. This is not a complete list. There might be further functions involved called from one of the listed functions:
obj_t
  ::display
  ::display_after
karte_t::sync_step,
objlist.cc compare_trees
world_view_t::internal_draw
baum_t
  ::finish_rd
  ::recalc_off
  ::rotate90
roadsign_t::set_dir
road_user_t::rdwr
air_vehicle_t::display_after
vehicle_t
  ::display_overlay
  ::make_smoke
  ::rdwr_from_convoi
  ::rotate90

There are a few more occurences that won't need changes.
« Last Edit: February 26, 2020, 10:45:25 PM by Freahk »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9729
  • Languages: De,EN,JP
Since you have millions of objects on large maps, and you break the packed alignment structure, you may suddenly end up with double memory consumption and slower code due to more cache misses. Furthermore, smoke_desc_t has the offsets as coordinates, which are sint16. So one could just spawn on the right tile (like supporting height as offset too in smoke_desc_t) and all things are solved. Nowhere wolke_t assumes to be on the map ground.

Code: [Select]
void fabrik_t::smoke() const
...
koord3d ro = rada->get_pos_off(size,rot);
grund_t *gr = welt->lookup(pos_origin+ro);
...

So smoke_desc_t::get_pos_off must return a koord3d instead of koord, which is rather trivial.

I will change the smoke_desc_t write tonight to used different 3d coordinates if there is an entry "smoketile[0]"=x,y,z and also use then up to four offsets, if "smokeoffset[0]" is given.

So the only change will be to smoke_desc_t::get_pos_off and handling of old style paks (which is like now) and new style, using an array of koord3d.

However, without seeing any code, this is totally blind guessing, and may be unfair to what you actually doing. (Which I have no idea).
« Last Edit: Yesterday at 05:45:22 AM by prissi »

Offline Vladki cz

  • Devotee
  • *
  • Posts: 2920
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Please wait with changes, I have almost finished patch fir exteded which could apply to standard too. I'll post later when I'm at pc. I tried to put the smoke at koord3d above ground but it was still shown on ground. Please look at related topic about factory smokes in extended forum

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9729
  • Languages: De,EN,JP
Kindly point me to the thread please. There is nothing obvious discussion this from the title. Anyway, I did it last night.

EDIT: Forgot the patch, which was only tested with old pak objects
« Last Edit: Today at 02:23:24 AM by prissi »

Offline Vladki cz

  • Devotee
  • *
  • Posts: 2920
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Please, see here: https://forum.simutrans.com/index.php/topic,17456.msg185173.html#msg185173
And the patch here: https://github.com/jamespetts/simutrans-extended/pull/81

I spend several nights debugging and fixing the smokes, and just didnt have time to announce it, And I was not at computer yesterday evening either.
I see your patch is quite extensive and I fear that all my work went down the drain... :-(

Edit: looking more at the patch, yeah - your and mine patch touch many of the same places with different apporach:

It is not enough to specify the tile with koord3d, the offsets have to be 3d as well for the rotations to work well.
And I failed when tried to modify the koord3d pos when creating smoke, it was still displayed on ground level (see my post above).

My patch also makes smokelifetime konfigurable (with default 2500/2499), and also makes the smokespeed to work (it was ignored before).
Further plans are to make wind speed (horizontal movement of the smoke) configurable.
« Last Edit: Today at 09:46:38 AM by Vladki »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9729
  • Languages: De,EN,JP
Since the graphics could be very different for different rotations (especially when there are only two rotations of a factory which is 2x2), the only way is to specify proper offsets. A single pair of x,y will always fail because it cannot guess correctly, which direction the smokestack goes in rotation. Hence the smoketile parameter. (And the same for smokeoffset.)

Configurable times for smoke can be added too, indeed. And the smokespeed too. But at least in standard the smoke speed (meaning how much smoke in old versions per time) is tied to the production rate of factories and the speed of vehicles.

However, currently the whole smoke system is somewhat broken, since the smoke is not use a independent smoke descriptor but rather something in between. Hence, I am very reluctant to add too much further hacks on it (which would be required by a configurable lifetime etc.)

So here a corrected version.
« Last Edit: Today at 02:28:11 PM by prissi »