Skip to content

Fix deprecations, correct return values #96

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

Merged
merged 17 commits into from
Apr 9, 2025

Conversation

NexiusTailer
Copy link
Contributor

@NexiusTailer NexiusTailer commented Apr 2, 2025

  1. Fix of some deprecated function names which were corrected by "upgrade" console command under the latest MTA SA server, but I think it can be updated in the repo itself by this moment
  2. Make the most of natives return true/false as a flag whether this native return success or it is failed to execute. Many SA-MP scripts logic depend on such behaviour because they use build-in validations in natives to omit manual validation of arguments which they've passed in those natives
  3. Fix the return values for GetPlayerSurfingVehicleID and GetPlayerSurfingObjectID, which are not -1, they're 65535 for an invalid vehicle or object ID (and when player is not surfing on anything)
  4. GetPlayerSurfingVehicleID now also has notImplemented function call since it actually just returns invalid vehicle ID value similar to GetPlayerSurfingObjectID, and there is no working implementation for actual surfing
  5. Fix the return values for GetPlayerCameraTargetObject, GetPlayerCameraTargetVehicle, GetPlayerCameraTargetPlayer and GetPlayerCameraTargetActor, which are not 0, they're 65535 for an invalid object, vehicle, player or actor ID
  6. Fix the return value for GetPlayerCameraMode which is -1 when the native is failed to execute (in our case it's not implemented, so it should always return a failure value)
  7. Make use of a_amx.inc by removing duplicated natives which already exist in SA-MP libraries (and thus give compile errors)
  8. Some corrections in readme file about newly added natives (GetVehicleVelocity and SetVehicleVelocity aren't new features)
  9. Make less confuse with many similar readme files (install.html in amx-deps and README.md) by removing legacy .html one
  10. Update original SA-MP libraries from official server package to their latest 0.3.7 version

@colistro123
Copy link
Collaborator

This PR contains multiple changes, and I will be reviewing it over the next few days. Thank you for your contribution! 😊

@NexiusTailer NexiusTailer requested a review from colistro123 April 4, 2025 01:12
Copy link
Collaborator

@colistro123 colistro123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I would definitely want to merge it.
I was wondering if the changes were properly tested since I don't have the environment setup for this at the moment. Are there any side effects? Thanks!

@NexiusTailer
Copy link
Contributor Author

NexiusTailer commented Apr 4, 2025

Sure, I'll make some additional tests covering as wide function list as possible and let you know if everything will be good (or make some additional tweaks if not)

@NexiusTailer
Copy link
Contributor Author

NexiusTailer commented Apr 9, 2025

After a long and comprehensive test I fixed a lot of things, mainly those which were present before my changes, but anyway I decided to extend the current pull request with them:

  1. All object related functions now validate if objectid passed to them is invalid and work correctly in this case
  2. SetPlayerPos and SetPlayerPosFindZ now remove player from the current vehicle before teleport like SA-MP does
  3. SetVehicleToRespawn now properly remove all players from vehicle before respawn it, as previously it worked incorrectly, teleporting players on vehicle spawn position and only then remove players from that vehicle
  4. OnVehicleSpawn now is called in every amx script like SA-MP does, previously it was called only from the current
  5. OnPlayerInteriorChange now is called only if player actually changed an interior (if newInt ~= oldInt only)
  6. ClearAnimations now remove player from the current vehicle like SA-MP does
  7. RemovePlayerFromVehicle now remove player from the current vehicle gracefully, with an exiting animation like SA-MP does
  8. OnPlayerExitVehicle no longer triggers when server removes a player from vehicle, since this callback should be only called when player press "Enter" to start exiting the vehicle by himself
  9. Fixed player states (OnPlayerStateChange) logic to be in line with SA-MP behaviour: the initial state is PLAYER_STATE_NONE when player just connected, also added 'PLAYER_STATE_SPAWNED' when player just spawned and before any onfoot update is called
  10. Fixed a typo in RemoveVehicleComponent with incorrect upgrade parameter (it's declared as upgradeID)
  11. Fixed a typo/mistake in PutBotInVehicle with incorrect oldwarpPedIntoVehicle function name (there's only warpPedIntoVehicle)
  12. Fixed ChangeVehicleColor behaviour: now this allows you to change colors for vehicles created with random color set (-1, -1) just like SA-MP does
  13. Fixed class selection behaviour when no classes are available (added) in any pawn script, now it spawns at 0.0, 0.0, 3.0 coordinates with CJ skin just like it happens on SA-MP server
  14. Fixed TogglePlayerSpectating function, it spawned players in the last position they went to spectate mode from, but in SA-MP they should be normally spawned like after death or SpawnPlayer function
  15. Fixed SpawnPlayer function, it didn't work because it always tried to call requestSpawn even if player has already spawned and wasn't in class selection. Now it works both in class selection and in any other case as well
  16. Fixed OnPlayerUpdate behaviour: it doesn't call when player not spawned (just connected, dead and if entered class selection). It's only called in the following states: onfoot, driver, passenger and spectating
  17. Fixed OnPlayerDeath behaviour: killerid must be another player (not vehicle or anything) and it should be INVALID_PLAYER_ID (65535) instead of 255 when a killer is invalid
  18. Set weapons skill levels to maximum by default like SA-MP does
  19. Set respawn time if vehicle was destroyed to 10 seconds like SA-MP does (in SA-MP, customizable respawn_delay only affects respawn delay for idle state after any player left this vehicle)
  20. Fixed a bug in OnPlayerPickUpPickup, trying to interact with the wrong element
  21. Fixed a bug when force return to class selection system inside onPlayerWasted, trying to interact with the wrong element
  22. Improved validations in client side scripts for camera and GUI related functions
  23. Additional validations for getVehicleMaxPassengers return value in loops which take it as the highest seat (vehicle with 0 seats like trailers make getVehicleMaxPassengers returning a not integer value, so now it's considered)
  24. Fixed WARNING: amx\client\samp_nametags.lua:2: File not found @ 'dxCreateFont' [client/arial.ttf] in client side scripts, now it's no more trying to re-create it. 'arial' is already added as default font and can be used unconditionally

Now I can say for sure it's stable and can be merged.

Copy link
Collaborator

@colistro123 colistro123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed your changes and they look good to me. If anything we can revert and correct. Thank you for your contribution, I will be merging this! ☺️

@colistro123 colistro123 merged commit ca0c9ee into multitheftauto:develop Apr 9, 2025
@NexiusTailer
Copy link
Contributor Author

Thank you so much. If you'll have some time, could you also solve #97 please? I tried to include only necessary things which could be done with little effort but major impact on how the installation will become more easier for any newbie who come here to try it

@colistro123
Copy link
Collaborator

colistro123 commented Apr 16, 2025

Update, it seems like the following is broken:
image

Ideally, this shouldn't appear when running my gamemode, hence why it was originally done that specific way to behave the same way SA-MP did.

In short, class selection now seems to show at all times, which seems wrong. 🤔

@NexiusTailer
Copy link
Contributor Author

Update, it seems like the following is broken: image

Ideally, this shouldn't appear when running my gamemode, hence why it was originally done that specific way to behave the same way SA-MP did.

In short, class selection now seems to show at all times, which seems wrong. 🤔

Could you share the minimal reproducible example of such code which causes it so I also can test?

@colistro123
Copy link
Collaborator

Update, it seems like the following is broken: image
Ideally, this shouldn't appear when running my gamemode, hence why it was originally done that specific way to behave the same way SA-MP did.
In short, class selection now seems to show at all times, which seems wrong. 🤔

Could you share the minimal reproducible example of such code which causes it so I also can test?

Sure, it's basically just this:

public OnPlayerConnect(playerid)
{
	TogglePlayerSpectating(playerid, true); // This gets later set to false when a skin from a dialog list is chosen
	return 1;
}

public OnPlayerRequestClass(playerid, classid)
{
	SpawnPlayer(playerid);
	SetPlayerSkin(playerid, 24);
	return 1;
}

@NexiusTailer
Copy link
Contributor Author

NexiusTailer commented Apr 16, 2025

Ok, seems I understand. The issue starts appearing when no classes was added and thus no spawninfo available. If this is the case, this code inside TogglePlayerSpectating is called and do the wrong thing. I already working on fix and it will be in the next PR very soon, thank you.

@NexiusTailer
Copy link
Contributor Author

@colistro123 can you test it again with changes from this tempo fork?
https://github.com/NexiusTailer/amx

Before I finish some other things and make a pull request with complete description, I would like to understand that now everything is definitely good regarding classes selection system

@colistro123
Copy link
Collaborator

colistro123 commented Apr 17, 2025

@colistro123 can you test it again with changes from this tempo fork? https://github.com/NexiusTailer/amx

Before I finish some other things and make a pull request with complete description, I would like to understand that now everything is definitely good regarding classes selection system

Thanks, I commented on your other commit (1) and (2).

It's been a while since commits were made to this repo and this isn't known information as of yet, but in the future (for now you can leave as-is) I would prefer it if PRs were broken down per feature. Since I believe it would make things easier to test and revert until it's on a working state if that's ok. 😊

i.e.
feat/onplayertakedamage, bugfix/class-selection, etc.

I appreciate all the contributions you have made, thank you!

@NexiusTailer
Copy link
Contributor Author

@colistro123 can you test it again with changes from this tempo fork? https://github.com/NexiusTailer/amx
Before I finish some other things and make a pull request with complete description, I would like to understand that now everything is definitely good regarding classes selection system

Thanks, I commented on your other commit (1) and (2).

It's been a while since commits were made to this repo and this isn't known information as of yet, but in the future (for now you can leave as-is) I would prefer it if PRs were broken down per feature. Since I believe it would make things easier to test and revert until it's on a working state if that's ok. 😊

i.e. feat/onplayertakedamage, bugfix/class-selection, etc.

I appreciate all the contributions you have made, thank you!

Sure. Sorry for the mess with the current approach, it's definitely better to split up every major (and probably even many minor) changes into separate PRs so it will be much easier to review and track it all in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants