@Basssiiie You might be interested

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

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?

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.

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. 🙂

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

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.

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.

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. 😅

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 😅

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?

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.

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. 😅

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;

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;

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? 😅

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");

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

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.

The API already provides the information for (10) in the RideVehicleObject.spriteGroups interface. It defines the number of sprites of the vehicle at various pitch angles. For example, the classic wooden coaster has 16 flat sprites, 4 slopes12 sprites, and 16 slopes25 sprites. Compare this to the regular wooden coaster which has 32 flat sprites, 4 slopes12 sprites, and 32 slopes25 sprites, or the RCT1 bobsled coaster which has 16 flat sprites, 4 slopes12 sprites, and 4 slopes25 sprites.

So vehicles could return a max that could be appropriate for that vehicle (as maybe defined in the object, in the future). Ideally in this scenario it should also return 16 as the max for that wooden coaster car you mentioned, I suppose (which I presume was the RCT1 "Classic Wooden Coaster"?).

Wouldn't the max just be numYawRotations per vehicle? I thought we would have it per vehicle anyway.

Considering your other points and now that the spin value also looks to be added, I can accept the yaw values to be 0-255 to have the consistency with the spinning range. I would suggest naming the step value yawStep though, as I'm not sure what "minimal" adds to the name.

Good! I am fine with yawStep.

And I'm wondering, should it take in account those vehicles with 16 or less rotation frames as well? Or always return 8 in this current implementation?

The question is, are we talking about rotation as a rendering property (i.e. frames) or as a physical property? I think it is the latter, which would mean that yawStep should be the precision of the internal data, and not dependendent on the number of sprites. Isn't it the case that the number of sprites depends on pitch and banking as well?

For setting, I don't really see any benefit having the yawStep reflect the internal increment precision if it doesn't match the visual increment precision, or am I missing something?

Pitch/bank have sprite fallback too of course, but they don't have a step property either.

For setting, I don't really see any benefit having the yawStep reflect the internal increment precision if it doesn't match the visual increment precision, or am I missing something?

Since you can read in-between values, it would be very confusing if yawStep does not reflect that. Imagine seeing yawStep = 32 but reading yaw as 16.

This pull request has been marked as stale and will be closed in 14 days if no action is taken. To keep it open, leave a comment or remove the stale-pr label. If you're awaiting feedback from a developer, please send us a reminder (either here or on Discord).

This pull request has been marked as stale and will be closed in 14 days if no action is taken. To keep it open, leave a comment or remove the stale-pr label. If you're awaiting feedback from a developer, please send us a reminder (either here or on Discord).

This pull request has been closed due to inactivity. If you want to continue with this, or are awaiting a review, don't hesitate to reach out to a developer. We'll gladly re-open this PR! You can tag us here, or send us a message on Discord.

This pull request has been marked as stale and will be closed in 14 days if no action is taken. To keep it open, leave a comment or remove the stale-pr label. If you're awaiting feedback from a developer, please send us a reminder (either here or on Discord).

This pull request has been closed due to inactivity. If you want to continue with this, or are awaiting a review, don't hesitate to reach out to a developer. We'll gladly re-open this PR! You can tag us here, or send us a message on Discord.

What's going on with this PR? I'll put in draft so we don't have to fight the bot.

I am still interested in seeing it merged. If help is needed, please let me know. 🙂

The interest from other people is part of why I reopened this. It also seems like it was waiting for a reply from us that didn’t come.

Only one year? Quite young for my PRs 😛

I'll see about setting my OpenRCT2 computer back up and dragging it across the line.

Hi @spacek531

Would you still be interested in rebasing and adapting this PR to the new JS engine? This contribution would be more than welcome. (I also still have a personal interest in this one.)

Thank you for your time and looking forward to hearing from you. 🙂