Linedef type 435 (Change Scroller Direction) has some issues

[Ricardo]

Before I begin, I have to say this looked more like an incomplete feature to me than a bug per se, so if that's the case, excuse me if you already figured these out.

The relevant code is found at http://trac.srb2.org/browser/SRB2/tags/SRB2v2.0.6/src/p_spec.c#L2450 but I'll paste it here anyway.

Spoiler

case 435: // Change scroller direction
	{
		scroll_t *scroller;
		thinker_t *th;

		for (th = thinkercap.next; th != &thinkercap; th = th->next)
		{
			if (th->function.acp1 != (actionf_p1)T_Scroll)
				continue;

			scroller = (scroll_t *)th;

			if (sectors[scroller->affectee].tag != line->tag)
				continue;

			scroller->dx = FixedMul(line->dx>>SCROLL_SHIFT, CARRYFACTOR);
			scroller->dy = FixedMul(line->dy>>SCROLL_SHIFT, CARRYFACTOR);
		}
	}
	break;

The first issue is that this just doesn't work with textures at all, only flats (if it was intentional then never mind).
I wonder if this line is the cause of it:

if (sectors[scroller->affectee].tag != line->tag)
	continue;

I mean, after all, the "affectee" is being checked as a sector and not as a sector or a line.

################################

The next issue revolves in the new scroll speed being scaled in a strange way. If you wish to change the direction of the scroll effect but keep the same speed you have to make this linedef around 10 times bigger than the one with the original scroll type. This wouldn't be much of a hassle if the carrying speed from the "Carry Object" types behaved in the same manner, instead it seems to be correctly scaled.
Maybe this is causing such distinction?

scroller->dx = FixedMul(line->dx>>SCROLL_SHIFT, CARRYFACTOR);
scroller->dy = FixedMul(line->dy>>SCROLL_SHIFT, CARRYFACTOR);

After all, the CARRYFACTOR value is only being used on the carrier scrollers. Perhaps this linedef could assign the new speed according to the scroller type?

################################

Finally, this is how LD530 (Scroll Floor Texture and Carry Objects) is being setup: http://trac.srb2.org/browser/SRB2/tags/SRB2v2.0.6/src/p_spec.c#L6309

Spoiler

case 530: // scroll and carry objects on floor
	for (s = -1; (s = P_FindSectorFromLineTag(l, s)) >= 0 ;)
		Add_Scroller(sc_floor, -dx, dy, control, s, accel, l->flags & ML_NOCLIMB);
	if (special != 530)
		break;

case 520:	// carry objects on floor
	dx = FixedMul(dx, CARRYFACTOR);
	dy = FixedMul(dy, CARRYFACTOR);
	for (s = -1; (s = P_FindSectorFromLineTag(l, s)) >= 0 ;)
		Add_Scroller(sc_carry, dx, dy, control, s, accel, l->flags & ML_NOCLIMB);
	break;

As you know, two scrollers are actually added, one for the texture and one for the conveyor belt. Notice the dx value is being set symmetrically between the two. Maybe, like the previous issue, the "Change Scroller Direction" line type should distinguish both kinds of scrollers and assign the correct dx value to each, because the way it is now it makes the texture and the conveyor belt to not always scroll in the same direction.

 

[AlamGBC]

you should use the tags/SRB2v2.0.6, not trunk or use the ?rev=#### to keep links alive to the correct place

 

[Ricardo]

Okay, it's done.