mtasa-blue

mtasa-blue copied to clipboard

Description

This PR adds the ability to get/set the plane's rotor, just similar to helicopters' rotor functions.

Summary

• combined the plane and heli to get/setVehicleRotorSpeed(), now this function will work on both heli and plane!
• added getVehicleRotorSpeed(vehicle) and setVehicleRotorSpeed(vehicle,speed)
• the function has exact identical behavior to the set/getHelicopterRotorSpeed(), has the same parameters and return type.
• This function also works on the jet plane, e.g. at-400, shamal, however, no visual effect on them, but will affect physics just like the heli does.
• the old set/getHelicopterRotorSpeed() is still there and working, I didn't touch that.

Sample code snippet

rotor.zip

Hope this PR fills the gap of rotor ralated things, now we got both heli & plane working :p

Nice work. Looking at the two functions, wouldnt it make sense to merge them? Eg get/setVehicleRotorSpeed

Although that involves deprecating and later removing the heli one which is probably a hassle

Nice work. Looking at the two functions, wouldnt it make sense to merge them? Eg get/setVehicleRotorSpeed

Although that involves deprecating and later removing the heli one which is probably a hassle

It's possible, but it will require deprecating the old one, so breaks the backward compatibility. Another possible way to do that is I can add heli detection into my get/setPlaneRotorSpeed() and combine them into one, so in the future MTA can properly deprecate the get/setHelicopterRotor(). idk, what you guys recommend?

Deprecating functions doesn't break backwards compatibility, as the only change would be that using the deprecated function shows a deprecation warning, and that's non-breaking.

I agree with @Zangomangu that that merging these functions in get/setVehicleRotorSpeed is nice, although it feels a bit weird that only two vehicle types will ever be able to have its rotor speed changed with that function. Anyway, there are functions like getVehicleLandingGearDown which only work on planes, so I think that the vehicleRotorSpeed naming is consistent at least.

Perhaps this is inconsistent, thus not a good idea, but wouldn't setAircraftRotorSpeed be more suitable since (from what I know) this only works for aircrafts?

Deprecating functions don't break backwards compatibility, as the only change would be that using the deprecated function shows a deprecation warning, and that's non-breaking.

I agree with @Zangomangu that merging these functions in get/setVehicleRotorSpeed is nice, although it feels a bit weird that only two vehicle types will ever be able to have their rotor speed changed with that function. Anyway, there are functions like getVehicleLandingGearDown which only work on planes, so I think that the vehicleRotorSpeed naming is consistent at least.

Sounds reasonable, I've update my PR to get/setVehicleRotorSpeed approach, now this function works on both plane & heli, the old set/getHelicopterRotorSpeed() still remains untouched, for backward compatibility.

Done, The PR is ready for review.

Perhaps this is inconsistent, thus not a good idea, but wouldn't setAircraftRotorSpeed be more suitable since (from what I know) this only works for aircrafts?

I think you've a point, and aircraft is a good word to choose, but so far it has only been used in the function names of getAircraftMaxHeight and getAircraftMaxVelocity, which are in the "world functions" category, not the "vehicle functions" category on the wiki. There are lots of get/setVehicleSomething that don't work on any vehicle type anyway, and it's conventional that functions that manipulate a vehicle element have the vehicle infix. Functions like get/setHeliBladeCollisionsEnabled and get/setHelicopterRotorSpeed are, coincidentally, the odd ones.

@AlexTMjugador, Done! upgrade to the new parser.

Nice! Is there any reason to keep the CStaticFunctionDefinitions::GetVehicleRotorSpeed and CStaticFunctionDefinitions::SetVehicleRotorSpeed methods, instead of doing their logic in CLuaVehicleDefs::GetVehicleRotorSpeed and CLuaVehicleDefs::SetVehicleRotorSpeed, respectively?

CStaticFunctionDefinitions

not really, I'm being a bit lazy Alright, I'm gonna do that do it now.

CLuaVehicleDefs

@AlexTMjugador Done! Removed the implementation from CStaticFunctionDefinitions and moved it all to CLuaVehicleDefs. Thanks for taking your time for the review, anything else?

Done! Refactor the rotor related code to use CPlaneSAInterface instead.

Two more things I'd like to ask before giving my symbolic approval to this PR:

• Did you run the clang code formatter on the PR? Use this script if not: https://github.com/multitheftauto/mtasa-blue/blob/master/utils/win-apply-clang-format.bat
• Have you tested that things continue to work well in-game after the refactors?

Two more things I'd like to ask before giving my symbolic approval to this PR:

• Did you run the clang code formatter on the PR? Use this script if not: https://github.com/multitheftauto/mtasa-blue/blob/master/utils/win-apply-clang-format.bat
• Have you tested that things continue to work well in-game after the reactors?

1. for the clang formatter , it pops up an error says Cannot find clang-format-r325576.exe, i guess it may not contain in the source code, so where do I obtain it then?
2. for reactors code testing, yes, I've tested it in my local machine, it behaves identically to the code before refactoring.

@AlexTMjugador alright, i've updated the changes according to the comments. please have a look :)

1. for the clang formatter , it pops up an error says Cannot find clang-format-r325576.exe, i guess it may not contain in the source code, so where do I obtain it then?

The comments in the format script point out where to get the needed executables. For the clang formatter try out this link, and for fnr.exe this one.

Just so you know for future, both Visual Studio and Visual Studio Code have support for formatting via the provided .clang_format file in the repo - simply use the relevant formatting shortcuts while writing code (if you don't want to use the .bat file).

• clang

Alright, I'll run through clang now

So anyone else wants to review this? perhaps it's being hanging up there for too long.

Looks like it doesn't compile?

last two commits don't work afaik

Build is failing.

Build is failing.

@patrikjuvonen fixed all the build problems & updated the PR to align with the latest MTA code, ready for review.

I think we should go ahead and deprecate the old helicopter functions. See how to deprecate functions over here: https://github.com/multitheftauto/mtasa-blue/blob/master/Server/mods/deathmatch/logic/CResourceChecker.Data.h#L444-L517

https://wiki.multitheftauto.com/wiki/SetHelicopterRotorSpeed and https://wiki.multitheftauto.com/wiki/GetHelicopterRotorSpeed will be marked as deprecated ??