-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API to ScVehicle for orientation and pitch #22048
base: develop
Are you sure you want to change the base?
Conversation
@Basssiiie You might be interested |
3999d7f
to
c67d1e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
b396f02
to
9058568
Compare
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 |
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.
|
I was thinking in 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 |
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 |
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 |
12d8613
to
e48beeb
Compare
|
In the 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 |
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.
|
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; |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I've tested it and I've got only one last nitpick. 🙂
ac0a871
to
ce50adb
Compare
|
ce50adb
to
e452826
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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. |
206849f
to
2dfd8a9
Compare
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. |
@@ -2507,10 +2579,26 @@ declare global { | |||
*/ | |||
velocity: number; | |||
|
|||
/** | |||
* The current value that represents one full revolution of the entity. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does 'current' mean in this context? This should not change in future API versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may change in the future, that's the whole reason the property is there, instead of not having it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your suggestions for allowing plugins to gracefully handle a change of base in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh this is a bit of a weird case. At a lot of places in the API, there are constants that influence the interpretation of values. We try to document them, but usually we do not assume that they might change and add an API value for that. E.g. guest happiness caps at 255, but there is no currentMaxGuestHappiness = 255
in the API. So I wonder why it is the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have not been talks of increasing the guest max happiness resolution. There have been talks of increasing the entity rotation resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it is suboptimal and @Gymnasiast also had concerns. But I also do not have the perfect solution either.
I don't know how we've lived this long without these properties.