The International Simutrans Forum

 

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

0 Members and 1 Guest are viewing this topic.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • 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

  • Devotee
  • *
  • Posts: 3723
    • 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

  • Devotee
  • *
  • Posts: 3723
    • 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

  • Devotee
  • *
  • Posts: 3723
    • 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

  • Devotee
  • *
  • Posts: 1523
  • 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: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #40 on: February 27, 2020, 12:31:14 AM »
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: February 27, 2020, 05:45:22 AM by prissi »

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #41 on: February 27, 2020, 04:21:42 PM »
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: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #42 on: February 28, 2020, 01:01:22 AM »
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: February 28, 2020, 02:23:24 AM by prissi »

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #43 on: February 28, 2020, 09:32:40 AM »
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: February 28, 2020, 09:46:38 AM by Vladki »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #44 on: February 28, 2020, 12:52:53 PM »
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: February 28, 2020, 02:28:11 PM by prissi »

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #45 on: February 28, 2020, 08:33:18 PM »
By smokespeed I mean how fast it rises up, not how often a new cloud is generated... Smokespeed 0 keeps the smoke on the same place so the artist can have full control of the movement.

Smoketime is good when adjusted to be the same as the duration of sound produced by the factory.

Smokeheigt is already hardcoded as 8 so I just made it configurable to make the rotation as precise as possible.

I know that even in rendered pak128.britain, some rotations are not exact, so the chimney and smoke are not always on the same place, but mostly it works fine.

For factories with 2 rotations, which are most probably just mirrored, a special rotation can be done.

If I understand your idea correctly you would prefer to specify the offset for each rotation separately and just make the smoke disappear on rotation?

I won't be able to compare your patch until Sunday evening, so please don't commit it. I spent quite a lot of time working on my patches and I would be quite disappointed if it would be useless.  Especially if your patch does not fix all what I fixed. Also I have already added the new options to several factories in pak128.britain and it looks really good. So I would like to keep that work... The only thing I need now is to overcome the height limit of sint8.

Also this is not only about factory smokes but also about vehicle smokes. They needed some fixes too.

I'm learning simutrans code this way so it takes me more time to find what's doing what. But when I find it I want to go in depth and prepare perfect patch.

I know that for some situations an animation would be better, that may follow later.

I also think about more smokes per factory...

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #46 on: February 29, 2020, 03:14:00 AM »
Smoke is currently the only object, which does not have its own descriptor, since it is derived from two smoke objects, which were just images. Therefore, the whole thing is a complete mess. There are two ways out: Either make this more messy be adding more parameter (like you did) or make a smoke its own descriptor, at least for factories. And I personally would much more prefer the latter. Like factory_smoke_t derived from wolke, which has all then all correct offsets and thus know on rotation to which tile it has to change to.

The old smoke was made so it worked with the available factories. New factories were added in such a way, that the smoke came out in the right position upon rotation. Any factory emitting wrong smoke was not made correctly (apart from overflow issues).

But, if one goes for renovation then up to four locations of smoke for each of the four rotations are mandatory. There is absolutely no rule stating that all rotations have to show the same factory (and some paks do not show exactly the same). So the only proper way of dealing with this is imho  having four different offsets. (That was actually even an extension request a while ago.)

However, if there are four different offsets, then the smoke would have to disappear upon rotation, as it cannot guess its position correctly. Or it will smoke from somewhere. (Which is currently the case with smoke on tiles != (0,0) and which is not easily fixable, even with your patch. And just drawing with larger offsets on the wrong base tile is also not helping, because then part of the building can be drawn on top of the smoke at certain rotations.

Offline makie

  • Devotee
  • *
  • Posts: 313
    • Homepage PAK128-German
  • Languages: DE
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #47 on: February 29, 2020, 08:07:31 AM »
But, if one goes for renovation then up to four locations of smoke for each of the four rotations are mandatory.
I think we need four images too. Think about a factory with two or more not centered chimney, may be a tiny and a big one. Then the image have to rotate too.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #48 on: February 29, 2020, 12:02:15 PM »
All the more for an independed object, or a changed definition of the factory descriptor.

(Originally, smoke was shared, but that would be almost impossible with such additions.)

Offline Leartin

  • Oh no, not him again!
  • Devotee
  • *
  • Posts: 1578
  • PAK-DEV P192C
  • Languages: DE, EN
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #49 on: February 29, 2020, 02:04:46 PM »
Shouldn't we also talk about which (if any) new parameters would be part of the smoke, and which would be parameters of the object spawning the smoke?

The way I see it, all a smoke object needs to be is an array of images displayed one after another as an animation, that can be spawned pretty much independently from everything else. If that's not fancy enough, a smoke could also display one of several animation whenever it spawns, or each frame of the smoke could be the result of a particle generator. Basically, no matter how fancy, the smoke object would define what is displayed *

The other thing is the definition when and where to spawn. For vehicles, that's hardcoded. For buildings, the dat needs to define it. But if we think of the smoke as just a displayed animation, and multiple animations per factory are already on the scope of what we want to do, I really recommend reading the proposal in this thread: https://forum.simutrans.com/index.php/topic,18388. (To recap: Define smokes exactly like images, since just like images, we only need to know which goes where)


Lastly... how about making this a completely new thing? If the old smoke just stays as it is, and this new system would be independent from it, there would not need to be any consideration towards backwards compatibility. Eg. Particles don't need to move up 5 pixels during their lifespan, because if you wanted them to do that, you'd always do it with the graphics. This means if you want the smoke to move up 7 pixels during it's lifetime, you could do that by redrawing it seven times, rather than 12 times **

I'm not sure, but would the images displayed over a building as a smoke require more processing than the same images displayed as a frontimage? If not, switching over to use smoke for tiny, repeated animations would be really useful!


* Just to be clear: That's completely independent. Even with no changes to smokes as they currently are, I don't see why you couldn't just reload new images created by a particle generator in the 'slots' of a normal smoke to make it completely random any time. Of course, the particle generator needs to be made and hooked up, but it's completely independent from this discussion.

** If someone can't follow: If you had a seven-frame smoke currently, where each frame moves up one pixel, after a fifth of the lifespan, it would move up. Since you don't want that, you have to manually define a different frame that looks the same, but is one pixel lower. This is the case for 5 of the seven frames, which are different images, hence 12 frames. This is assuming the game is smart enough not to redraw the smoke if it neither moved nor got replaced with a different image; otherwise a seven-frame-smoke would require 35 redraws.

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #50 on: March 02, 2020, 09:25:40 PM »
OK, I agree that having separate position and offset for each rotation is better...

For what leartin says about the animations - in my patch I have added two options:
smokespeed - it says how fast the smoke moves up, with 0 - does not move at all, and 1 the current speed
smokelife - how long the smoke lives with 2500 being the current hardcoded default.
Also if you make the animation with more than 5 images, they will change even if the smoke does not move up.
I has a discussion with Jamespetts whether smokespeed (and smokelife) should be bound to smoke or to factory.
Smokespeed should be imho defined together with smoke, but for smokelife it is good to match the duration of sound emitted by factory.

Prissi, could you post an example how a dat file for factory with smoke should look like with your patch? I'll try to apply it to some factories in pak128.britain to see how it works.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #51 on: March 02, 2020, 11:47:58 PM »
I think positions should be bound to factories. A derived factory_wolke_t from wolke_t could then just (upon rotation) look for a factory on its tiels, and then determine its new tile after rotation from its descriptor. (Same for lifetime, unique to factory_wolke_t.)

About smookespeed: Both options are fine, i.e. a dedicated parameter or not.

Having offsets as part of the factory would be even better for reusing the smoke uopn different buildings.

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #52 on: March 03, 2020, 07:55:33 AM »
So wolke_t would be the smoke from vehicles?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #53 on: March 03, 2020, 12:59:33 PM »
wolke_t would stay like it is (or maybe renamed to smoke_t at this time), and a new factory_smoke_t is derived which uses the offset system for drawing the smoke and speed and lifetime and more checks for rotation.

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #54 on: March 03, 2020, 09:49:48 PM »
Prissi, please could you show an example how the dat file of factory with smoek should look like with your patch? Is that already with different offsets for different rotations?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #55 on: March 04, 2020, 01:49:30 PM »
The dat file would look the same, but the smoke offsets will be stored with the factory and the smoke offsets of the smoke will be ignored. If this is an old factory pak, then after loading the smoke offsets will be copied from the smoke descriptor.

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #56 on: March 04, 2020, 08:53:11 PM »
Sorry I do not understand... What is your diff then doing? Only allowing y_offset to be sint16 ? Adding different offsets for  rotations ? How are they coded? So again How do I code smoke for factory that has very high chimney and 4 rotations ?

OK, I think I got it...   4 rotations: smoketile[0-3] and smokeoffset[0-3], and 16 bit y_offset...   I had to do some modifications:

in factory_writer:
Code: [Select]
- if( !obj.get( smoketile ) )
+ if( obj.get( smoketile ) == "" )
...
- if( obj.get( smoketile ) )
+ if( obj.get( smoketile ) != "" )

Also the write offsets were wrong:
Code: [Select]
                // now we know the size
-               obj_node_t node( this, 3 + (4 + 4)*rotations, &parent );
+               obj_node_t node( this, 3 + 8*rotations, &parent );
                node.write_sint16( outfp, version, 0 );
                node.write_sint8( outfp, rotations, 2 );
                for( int i = 0; i < rotations; i++ ) {
-                       node.write_sint16( outfp, st[ i ].x, 5 + 9*i     );
-                       node.write_sint16( outfp, st[ i ].y, 5 + 9*i + 2 );
-                       node.write_sint16( outfp, so[ i ].x, 5 + 9*i + 4 );
-                       node.write_sint16( outfp, so[ i ].y, 5 + 9*i + 6 );
+                       node.write_sint16( outfp, st[ i ].x, 3 + 8*i     );
+                       node.write_sint16( outfp, st[ i ].y, 3 + 8*i + 2 );
+                       node.write_sint16( outfp, so[ i ].x, 3 + 8*i + 4 );
+                       node.write_sint16( outfp, so[ i ].y, 3 + 8*i + 6 );


And found the weird condition in rotation90 in simobj.cc, is needed for bridges. With this patch, bridges (the ramps) get broken, at least in simutrans-extended. But we do not need that if the smokes are removed upon rotation.
« Last Edit: March 05, 2020, 12:25:22 AM by Vladki »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #57 on: March 08, 2020, 03:08:35 AM »
Bridges were broken, that hack in rotation was just a catastrophy wating.

I submit my changes together with your ideas and input about speed going upwards i r8948. This allows now to have a smoketile[0] and smokeoffset[0] (and further roations) parameter, to have smoke coming out at different locations, as well as an smokeuplift and smokelifetime which are default 16 and 2599.

Thank you very much, without your work this would have never been finished.

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #58 on: March 08, 2020, 07:57:20 PM »
Thanks, I'll try to backport (or forward-port) this to extended.  BTW with all the changes now, would it be too complicated to have multiple smokes per factory? In pak128.britain, there are several factories with multiple chimneys.  I'm not too configent in C++ so I don't know what would be the "right way". I see that the rotations are stored as vector, so would it then be a vector of vectors?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #59 on: March 09, 2020, 01:23:15 AM »
This is a little more complicated, since the smokes are references semi-automatically resolved by the ref reader. But in principle, yes, there could be four different smokes per rotation (but more than one smoke per factory does no make sense, rather making a special smoke for that factory makes sense).

Offline Leartin

  • Oh no, not him again!
  • Devotee
  • *
  • Posts: 1578
  • PAK-DEV P192C
  • Languages: DE, EN
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #60 on: March 09, 2020, 01:30:03 PM »
(but more than one smoke per factory does no make sense, rather making a special smoke for that factory makes sense).
Could you explain why?
One of the best parts about smokes (and why I'd wish for them in non-factory buildings) is that they are reusable. Sure, if there are, say, two chimneys, I can make a smoke with graphics of two smokes. But then, I'd need to use four of those if the building with two chimneys rotates, and that smoke could not be reused in other factories, so I'd start questioning why it would even be a seperate object.
It makes more sense to me that each smoke would have the same 'graphical origin', that is, if smokeA is replaced with smokeB, it's a different graphic, but still looks like it comes out of the chimney, not 20px left from it. I'd think smokes should be independent from specific buildings whenever possible.

Offline Freahk

  • Devotee
  • *
  • Posts: 1523
  • Languages: DE, EN
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #61 on: March 09, 2020, 01:44:53 PM »
Without knowing the code, I do in principle agree with Leartin. Multiple smokes per industry make much more sense than a single smoke per factory that might be a factory dependant "multismoke" frames.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #62 on: March 09, 2020, 11:36:43 PM »
One can make smoke a part of the building, instead of only a factory without too much work and not introducing too much hacks. But having more than one smoke per building/factory and rotation will be quite some effort, which I doubt to ever see used in a real game for more than one object. (At least comparing to just copy smoke together into one image.)

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #63 on: March 22, 2020, 07:14:06 PM »
Hello Prissi,

I have tried the new version, but there is a problem in descriptor/writer/factory_writer.cc (I wrote about it earlier).

The test for new/old style of specifying smokes is using:
Code: [Select]
if(  obj.get( str_smoketile )  )But in the definition in dataobj/tabfile.h says that:          * @return const char * returns at least an empty string, never NULL.
So the test is always true, and thus old style smoke definition (which is OK for factories wit only one rotation), does not work - smoke is generated at tile 0,0, offset 0,0
The needed change is either:

Code: [Select]
--- a/descriptor/writer/factory_writer.cc
+++ b/descriptor/writer/factory_writer.cc
@@ -277,11 +277,11 @@ void factory_writer_t::write_obj(FILE* fp, obj_node_t& parent, tabfileobj_t& obj
        sint16 const smokelifetime = obj.get_int("smokelifetime", DEFAULT_FACTORYSMOKE_TIME );
        char str_smoketile[] = "smoketile[0]";
        char str_smokeoffset[] = "smokeoffset[0]";
-       if(  obj.get( str_smoketile )  ) {
+       if(  obj.get( str_smoketile ) != ""  ) {
                for( int i = 0; i < 4;  i++  ) {
                        str_smoketile[10] = '0' + i;
                        str_smokeoffset[12] = '0' + i;
-                       if( !obj.get( str_smoketile ) ) {
+                       if( obj.get( str_smoketile ) == "" ) {
                                break;
                        }
                        pos_off[ i ] = obj.get_koord( str_smoketile, koord( 0, 0 ) );
-                       if( !obj.get( str_smokeoffset ) ) {
+                       if( obj.get( str_smokeoffset ) == "" ) {
                                dbg->error( "factory_writer_t::write_obj", "%s defined but not %s!", str_smoketile, str_smokeoffset );
                        }
                        pos_off[ i ] = obj.get_koord( str_smoketile, koord( 0, 0 ) );


Or, as I observer in other parts of code:

Code: [Select]
--- a/descriptor/writer/factory_writer.cc
+++ b/descriptor/writer/factory_writer.cc
@@ -277,15 +277,15 @@ void factory_writer_t::write_obj(FILE* fp, obj_node_t& parent, tabfileobj_t& obj
        sint16 const smokelifetime = obj.get_int("smokelifetime", DEFAULT_FACTORYSMOKE_TIME );
        char str_smoketile[] = "smoketile[0]";
        char str_smokeoffset[] = "smokeoffset[0]";
-       if(  obj.get( str_smoketile )  ) {
+       if(  *obj.get( str_smoketile ) ) {
                for( int i = 0; i < 4;  i++  ) {
                        str_smoketile[10] = '0' + i;
                        str_smokeoffset[12] = '0' + i;
-                       if( !obj.get( str_smoketile ) ) {
+                       if( ! *obj.get( str_smoketile ) ) {
                                break;
                        }
                        pos_off[ i ] = obj.get_koord( str_smoketile, koord( 0, 0 ) );
-                       if( !obj.get( str_smokeoffset ) ) {
+                       if( ! *obj.get( str_smokeoffset ) ) {
                                dbg->error( "factory_writer_t::write_obj", "%s defined but not %s!", str_smoketile, str_smokeoffset );
                        }
                        pos_off[ i ] = obj.get_koord( str_smoketile, koord( 0, 0 ) );

Or as used in way_writer.cc:         
Code: [Select]
if (!strempty(obj.get(buf))) { ...
UPDATE:
Also the vehicle smokes are broken after rotation. I tried to put back part of the original code, which helped on straight track, but it was still broken when the train was going up- or down-hill. It is impossible to get the rotation right without proper 3D offsets (x_off, y_off, h_off), as was implemented in my proposed patch. So if 3D offsets are not wanted, we can discard the smoke after rotation altogether.  If the vehicle smoke after rotation is wanted, then I can prepare a patch for it.

Also if the game is paused and rotated, the industry smoke IS rotated and stays in place until cursor goes over it.   

Another question is about the calls to mark_image_dirty() in wolke.cc. Shouldn't the second parameter (yoff) be always -8  to match with set_yoff( -8 ); from constructor? It seems that set_yoff is not used anywhere else, as the movement is done in display_after().

Also the new_yoff calculation in sync_step is imho useless, it works fine like this:
Code: [Select]
@@ -116,14 +116,10 @@ sync_result wolke_t::sync_step(uint32 delta_t)
                return SYNC_DELETE;
        }
        const image_id new_img = get_front_image();
-
-       // move cloud up
-       const sint16 new_yoff = base_y_off - (((long)insta_zeit * uplift * OBJECT_OFFSET_STEPS) >> 16);
        if(  new_img != old_img  ) {
-               // move/change cloud ... (happens much more often than image change => image change will be always done when drawing)
                if(  !get_flag( obj_t::dirty )  ) {
                        set_flag( obj_t::dirty );
-                       mark_image_dirty( old_img, new_yoff );
+                       mark_image_dirty( old_img, -8 );
                }
        }
        set_flag( obj_t::dirty );




« Last Edit: March 22, 2020, 10:00:24 PM by Vladki »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #64 on: March 23, 2020, 07:52:34 AM »
The last bit will not work with long raising clouds, because only the base is marked dirty, not the raidsing cloud. I may work in your tests, since the dirty boxes are larger than the clouds. It was not also not noted so much, because even non-moving clouds were dirty all the time.

I tested mainly 64 size pak, and the vehicle smoke was fine for three out of four rotations. Since more smoke (for steam engines) is better than less, I just left it in. It is possible, that vehicle smoke in pak128 works less after rotation. (In pak128.britain is seems to cope even better with rotation.) But the smoke is only shown for a second or so after rotation before it fades anyway. So I decided this a non serious problem. Better than no smoke at least.

I mean rotation is something almost not player does frequently (or in networkgames never) and thus I would not bother too much. One could indeed move the industrysmoke to the right tile, but that will be a big hassle, since it cannot be done during the rotation, but only during hte next sync_step. (Since we cannot alter the tile during rotation! That is why the smoke stay when paused.)

about obj.get, sorry, that is indeed my fault. Corrected in r8966 (and the dirty offsets in r8967)

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #65 on: March 23, 2020, 10:10:01 PM »
Hi, I have noticed, that sometimes the last sprite of smoke stays in place. Soot falling on the ground...

I tried to add yoffset in to the mark_image_dirty() in destructor  ~wolke_t(), but that did not help. I also added mark_image_dirty() call just before return SYNC_DELETE. That seems to be better, but occasional leftowers still happen

As the uplift calculation is already 3x in the code, I made a method calc_yoff, that does this calculation.
Further I found a bug in factory_writer.cc, that ignored offsets[0-3]. And some typos and fixes in comments
And as last I added a fix for smoke rotation to make it work perfectly at least on flat track.

Please see the attachment

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #66 on: March 27, 2020, 10:37:19 PM »
I did further work on this.

0. xy_offsets were ignored due to a typo in factory_writer.cc
1. implemented perfect rotation for vehicle smokes - this requires 3D offsets, which are easy for vehicles. Factory smoke is discarded on rotation so there is no need to change anything.
2. fixed the mark_image_dirty calls (yoff must be scaled to tile_raster), so now the smokes properly disappear
3. removed the randomization of initial position of smoke
4. added a "wind" effect - the clouds move randomly sideways
5. some comment fixes

New patch attached, please ignore the previous one.
EDIT: screenshot of the "wind effect - #3 and #4"

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #67 on: March 28, 2020, 12:47:46 PM »
Also the obj better rotates, even for factory smoke, otherwise it removal from the map may fail (if a deletion happens before the next step, i.e. roation after pause).

About the wind, there is more discussion needed. 2 pixel may be a lot for smaller smoke. And offset must be changed after marking dirty, or remainder of clouds are left.

Incorporated in r8980, thank you.
« Last Edit: March 28, 2020, 01:02:31 PM by prissi »

Offline Vladki

  • Devotee
  • *
  • Posts: 3723
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #68 on: March 28, 2020, 01:29:19 PM »
About the wind, there is more discussion needed. 2 pixel may be a lot for smaller smoke.

rand(2) returns either 0 or 1, and it is invoked only on yoff changes, so the wind is never too strong

And one more question, I'd like to get rid of that hard coded yoff=-8, at least for new factory smokes, so that the offset can be directly measured from the png image. Is that OK?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10674
  • Languages: De,EN,JP
Re: Changes to the smoke-parameter for buildings (and ideas around that)
« Reply #69 on: March 28, 2020, 02:22:07 PM »
If you can do it so the old is still intact, why not. But the offset is not in pixel anyway (unless you have 128 size).