Add API to ScVehicle for orientation and pitch

[spacek531]

I don't know how we've lived this long without these properties.

[spacek531]

@Basssiiie You might be interested

[Basssiiie]

Thanks for this PR! It's something I wanted added myself but never got around to doing it myself. Glad you got around to it. 😄

[spacek531]

Would it make sense to change the two new types CarPitch and CarBank to enums?

[Basssiiie]

Sorry for the late reply. Regarding enums or string unions.. While I'd normally prefer string unions, wouldn't it better here to just use and expand TrackSlope and TrackBanking? It seems they are already using the same numbers. It would also allow compatibility between the two fields.

In regards to the rotation/yaw setting. I'm not really a fan of making it 256, as now it's not at all clear by how much you'll need to increment it before it does anything at all. Maybe an idea to keep it at a rotation property with the range of 0-31 along with a readonly property maxRotationDegrees that always returns 32 for now?

[spacek531]

Where would you put MaxRotationDegrees? In ScEntity, ScVehicle etc., or a global? "How much you'll need to increment it before it does anything at all" is different per entity and per vehicle it's different for each vehicle, since they define how many rotation sprites they have per pitch angle. So it's a complicated question.

There's some groundwork that can be done with respect to using the TrackSlope and TrackBanking enums which I will do in a different PR because it's a lot of re-naming things. I just realized that I don't have to do that because the enum return type is an int not a string.

[Basssiiie]

I was thinking in ScVehicle because the other ones do not have a rotation property anyway.

Regarding renames of enums. I prefer to keep breaking changes in renames to a minimum, preferably avoided. Whether that is in this PR or another, I leave that's up to you. 🙂

[spacek531]

The others do have a rotation property, it's a part of EntityBase.

[Basssiiie]

I meant they don't have one exposed in the plugin API. And for some entities it doesn't do anything, so exposing it via ScEntity seems overkill from an API perspective.

[spacek531]

If it isn't exposed in EntityBase then it has to be exposed individually for all the entity types it does affect. So it's a question of which is less desirable: exposing API properties that do nothing for some entity types, or duplicate code bloat for the entities that do have usable rotation. I would say code bloat is less desirable.

[Basssiiie]

From the API perspective, having properties that do not do anything for certain types is confusing and unnecessary bloat. IMHO that's like having a text property on WidgetBase because about half of the widgets use it, even though the other half don't. Internally you can of course share code/implementation, but for plugin users it's only confusing if they have to deal with these internal implementation gotcha's. 😅

[spacek531]

How would you suggest sharing implementation? Is there a way to call ScEntity::rotation_get() from ScVehicle::Register()? I would not have to define rotation_get() and rotation_set() for every entity type? I just made the getters and setters public and it worked 😅

[Basssiiie]

In the text example they're implemented as protected: ScWidget.

How would that work for guests vs. vehicles? I'd assume guests and staff have only rotations 0-3 right? Would that work with the same implementation? I'd assume the maxRotationDegrees has to be different right?

[spacek531]

Regarding making the rotation input range 0-31 and having a "max rotation degrees" value, I think that is less elegant than making the rotation 0-255 and having a "rotation degree interval" value (which would be 8 for vehicles and 64 for peeps). With the former, plugins have to manage reading that and doing conversions for every read/write. With the latter, plugins can read/write the value directly with no annoying conversion bloat, and choose to use a higher precision if they desire with the caveat that the rotation may not be rounded internally in the future which would change the drawn angle.

[Basssiiie]

Don't plugin developers need to do conversions in both cases? 0-255 is just technical rotation, not intuitive/logical like 0-360 for display. 😅 The only benefit is that they're the same range I guess. I don't know how often people want to set a cars rotation to be the same as a guest though.

Edit: for the RideVehicleEditor for example, I would prefer to just convert the 0-31 range to proper 0-360 degrees for display, and then convert back to 0-31 for setting.

Edit 2: also, tile elements are already 0-3 for rotation, so 0-255 still wouldn't make everything consistent. 😅

[spacek531]

The specific use case I am designing for is a plugin that reads user-defined animation data packed in the park and plays it back based on user-defined triggers. For example, an animation can make a vehicle point in a specific direction. With rotation always being 256, the plugin can read the rotation and write it directly to the vehicle. With rotation changing between 32 and 64, the data then has a type that has to be stored ("is the data representing 32 or 64 or 256 degrees?") and convert accordingly. It would be nice not to have that problem.

The other element that I haven't mentioned is, if entity rotation is increased from 32 to 256, it will match the vehicle spinning property which is a 256-degree value, which will likely make spinning less complicated.

I thought about what implementing either method would involve for the RVE. I'm imagining a spinbox that calls a function that increments or decrements by the correct amount to advance the rotation exactly 1 sprite. The difference between having a 32 or 256 max degrees is the numerator.

spriteGroup = getSpriteGroupFromPitch(rideObjectVehicle, pitch); // implementation is outside of scope of example
numSprites = spriteGroup.spriteNumImages;
incrementValue = GetVehicleNumDegrees() / numSprites;
targetVehicle.rotation += incrementValue;

spriteGroup = getSpriteGroupFromPitch(rideObjectVehicle, pitch);
numSprites = spriteGroup.spriteNumImages;
incrementValue = 256 / numSprites;
targetVehicle.rotation += incrementValue;

[Basssiiie]

With rotation changing between 32 and 64, the data then has a type that has to be stored ("is the data representing 32 or 64 or 256 degrees?") and convert accordingly.

No need for that, this works for all degree differences and ranges:

var to256 = (car.rotation * 256) / car.maxRotationDegrees;
var from256 = (input * car.maxRotationDegrees) / 256;

It will also survive any degree increases on the C++ side.

For RVE I would probably use a spinner yes and the following code:

var to360 = (car.rotation * 360) / car.maxRotationDegrees;
var from360 = (input * car.maxRotationDegrees) / 360;

[spacek531]

In the text example they're implemented as protected: ScWidget.

How would that work for guests vs. vehicles? I'd assume guests and staff have only rotations 0-3 right? Would that work with the same implementation? I'd assume the maxRotationDegrees has to be different right?

I thought I knew how to do this based on the example but CI isn't having it and I don't know why. Can you dissect? 😅

[Basssiiie]

Maybe because it's missing the explicit template arguments? Like this:

// Explicit template due to text being a base method
dukglue_register_property<ScButtonWidget, std::string, std::string>(
    ctx, &ScButtonWidget::text_get, &ScButtonWidget::text_set, "text");

[spacek531]

I'm not convinced that 0-31 is forwards-thinking but I want this merged soon so I have changed it.

[Basssiiie]

LGTM! I've tested it and I've got only one last nitpick. 🙂

[spacek531]

I thought that the G1 icon names PR would bump API so this PR bumps API twice. This isn't a bad thing if both this and #22046 are approved to merge at the same time - I believe merging that first and this second will work without rebasing and properly increment API each time. It will still conflict with the other's changelog entry so, sadly, it's not 100% ready to merge right after the other.

[Gymnasiast]

I'm not convinced that 0-31 is forwards-thinking but I want this merged soon so I have changed it.

This kind of thing concerns me. We have already added too much shit we still have to support, I’m reluctant to add stuff that we already consider to be less than ideal before it’s even merged

[Basssiiie]

@Gymnasiast As mentioned on Discord, it's not a 0-31 system but a 0-max system where the max is determined by another property. This allows us to upgrade it per vehicle or globally at a later time without breaking plugins, and avoids noise by adding useless values in between the useful values.

[spacek531]

I've thought about the merits of both methods. What made my decision was this:

var rotation = car.yawRotation;
while (car.yawRotation == rotation)
{
    car.yawRotation++;
}

In 32 mode (PR's current configuration), this works fine. In 256 mode, this will hang because the internal value rounds down to the nearest 8. I can't argue against this case, so I agree with Basssiiie that this is the better method.

[Sadret]

It would be nice to be consistent here regarding the naming convention and types. I.e. why is rotation a number but the others are enums? Why is the type of 'pitch' called 'Slope' but the type of 'bank' called 'Banking'?
Ofc existing enums should not be changed (changing the name of non-enum types is not a problem though btw).

[spacek531]

Yaw is a number because it is ordered - if you add 1 to your yaw, you will turn a specific amount every time. Roll and pitch are enums because they are out of order. You can see the order in the TrackSlope and TrackBank enum.

The normal way 3-axis rotations are described is "roll" "pitch" "yaw" but OpenRCT2 does not use this convention. So it is natural to use "roll" and "pitch" instead of "bank" and "slope." I caught the roll one but didn't catch the pitch one.

[spacek531]

I've changed some of the names with the goal of making the name more consistent across the codebase. Older parts of the code use bank and slope and newer parts use roll and pitch, so I've adjusted things accordingly.

[Basssiiie]

I was more a fan of the original implementation. It seems bloat to have the enum defined twice just so naming is consistent internally (but inconsistent in plugin API land).

As mentioned before though, I'm not a fan of the roll/yaw/pitch implementation internally either because it's applying the roll/yaw/pitch system to something that isn't working like that system. It's basically a lie about how it works. But that's a discussion for outside this PR.

[Sadret]

What does 'current' mean in this context? This should not change in future API versions.

[spacek531]

It may change in the future, that's the whole reason the property is there, instead of not having it.

[Sadret]

I don't like that. If a plugin wants to save the yaw across sessions, it now also needs to save numYawValues and figure out what to do if this value changes in the future.

[spacek531]

What are your suggestions for allowing plugins to gracefully handle a change of base in the future?