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

Function setPedWearingJetpack #243

Merged
merged 20 commits into from Jul 31, 2018

Conversation

Projects
5 participants
@Dezash
Contributor

Dezash commented Jul 19, 2018

setPedJetPack(ped thePed, bool state)
OOP: ped:setJetPack(bool state)

Two functions for the same purpose (givePedJetPack and removePedJetPack) isn't very convenient. Devs should consider deprecating them. I didn't do that because there seems to be no proper way to /upgrade old scripts as setPedJetPack requires an extra argument.

Dezash added some commits Jul 19, 2018

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

Merging this will need some thought.

If you want to submit a pull request to have ped.jetpack = bool, I can merge that straight away.

If you would like to do that, see ped.vehicle:

Member

qaisjp commented Jul 19, 2018

Merging this will need some thought.

If you want to submit a pull request to have ped.jetpack = bool, I can merge that straight away.

If you would like to do that, see ped.vehicle:

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

Apologies if I wasn't clear! If you can add ped.jetpack (and only ped.jetpack) to a different pull request, I can merge that pull request straight away.

Member

qaisjp commented Jul 19, 2018

Apologies if I wasn't clear! If you can add ped.jetpack (and only ped.jetpack) to a different pull request, I can merge that pull request straight away.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 19, 2018

Contributor

Why? Not everyone does OOP. This PR makes it possible to control JetPack in 3 different ways.

addCommandHandler("jp", function(thePlayer, _, state)
	setPedJetPack(thePlayer, state == "true")
	print(doesPedHaveJetPack(thePlayer))
end)

addCommandHandler("jpvar", function(thePlayer, _, state)
	thePlayer.jetpack = (state == "true")
	print(thePlayer.jetpack)
end)

addCommandHandler("jpmethod", function(thePlayer, _, state)
	thePlayer:setJetPack(state == "true")
	print(thePlayer:doesHaveJetpack())
end)
Contributor

Dezash commented Jul 19, 2018

Why? Not everyone does OOP. This PR makes it possible to control JetPack in 3 different ways.

addCommandHandler("jp", function(thePlayer, _, state)
	setPedJetPack(thePlayer, state == "true")
	print(doesPedHaveJetPack(thePlayer))
end)

addCommandHandler("jpvar", function(thePlayer, _, state)
	thePlayer.jetpack = (state == "true")
	print(thePlayer.jetpack)
end)

addCommandHandler("jpmethod", function(thePlayer, _, state)
	thePlayer:setJetPack(state == "true")
	print(thePlayer:doesHaveJetpack())
end)
@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

The reasoning is that merging this pull request needs some thought! There is no jetpack variable at the moment so a pull request to add just that variable is trivial to merge.

Does that make sense?

Member

qaisjp commented Jul 19, 2018

The reasoning is that merging this pull request needs some thought! There is no jetpack variable at the moment so a pull request to add just that variable is trivial to merge.

Does that make sense?

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 19, 2018

Contributor

Not really, would you mind sharing those thoughts? Also, this variable wouldn't work without setPedJetPack function. You can even see a commented out line attempting to add it with givePedJetPack and removePedJetPack, but it didn't seem to work out.

Contributor

Dezash commented Jul 19, 2018

Not really, would you mind sharing those thoughts? Also, this variable wouldn't work without setPedJetPack function. You can even see a commented out line attempting to add it with givePedJetPack and removePedJetPack, but it didn't seem to work out.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

Not really, would you mind sharing those thoughts?

I generally agree that replacing the current two functions with setPedWearingJetpack is a good idea. This would need discussion with the other devs.

I am not sure if we should merge this for 1.5.6, and if we do, it should probably be implemented in a different way (instead of creating a new packet, we should utilise the old two packets — we can replace those with one packet in 1.6).

If you are burning for this feature, I can merge the ped.jetpack variable change now. Otherwise the other changes and discussions need to happen. Does that help?

Also, this variable wouldn't work without setPedJetPack function. You can even see a commented out line attempting to add it with givePedJetPack and removePedJetPack, but it didn't seem to work out.

An implementation similar to ped.vehicle is required, it's not a 1 line change. See my previous comment.

Member

qaisjp commented Jul 19, 2018

Not really, would you mind sharing those thoughts?

I generally agree that replacing the current two functions with setPedWearingJetpack is a good idea. This would need discussion with the other devs.

I am not sure if we should merge this for 1.5.6, and if we do, it should probably be implemented in a different way (instead of creating a new packet, we should utilise the old two packets — we can replace those with one packet in 1.6).

If you are burning for this feature, I can merge the ped.jetpack variable change now. Otherwise the other changes and discussions need to happen. Does that help?

Also, this variable wouldn't work without setPedJetPack function. You can even see a commented out line attempting to add it with givePedJetPack and removePedJetPack, but it didn't seem to work out.

An implementation similar to ped.vehicle is required, it's not a 1 line change. See my previous comment.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 19, 2018

Contributor

I'm not burning for this feature and I think that replacing the other two packets is a great idea.

Contributor

Dezash commented Jul 19, 2018

I'm not burning for this feature and I think that replacing the other two packets is a great idea.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 19, 2018

Contributor

Here are some of the ideas that I thought of:

  1. Change setPedJetPack to use GIVE_PED_JETPACK and REMOVE_PED_JETPACK packets.
  2. Change give/removePedJetPack to use SET_PED_JETPACK packet.
  3. Use all 3 function and packets.
  4. Scrap setPedJetPack and add OOP function like the vehicle class.
  5. 3 and deprecate give/removePedJetPack in 1.5.6. Remove them completely in 1.6 to get rid of the extra packets.
  6. ???
Contributor

Dezash commented Jul 19, 2018

Here are some of the ideas that I thought of:

  1. Change setPedJetPack to use GIVE_PED_JETPACK and REMOVE_PED_JETPACK packets.
  2. Change give/removePedJetPack to use SET_PED_JETPACK packet.
  3. Use all 3 function and packets.
  4. Scrap setPedJetPack and add OOP function like the vehicle class.
  5. 3 and deprecate give/removePedJetPack in 1.5.6. Remove them completely in 1.6 to get rid of the extra packets.
  6. ???
@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

Let's do this:

Now:

  • Rename this function to setPedWearingJetpack (the different capitalisation of Jetpack is deliberate)
  • Use the existing packets (remove your packet code)
  • Mark the old two functions (give/remove) as deprecated

Later, in a different pull request, for 1.6:

  • replace with a single packet

What do you think?

Member

qaisjp commented Jul 19, 2018

Let's do this:

Now:

  • Rename this function to setPedWearingJetpack (the different capitalisation of Jetpack is deliberate)
  • Use the existing packets (remove your packet code)
  • Mark the old two functions (give/remove) as deprecated

Later, in a different pull request, for 1.6:

  • replace with a single packet

What do you think?

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 19, 2018

Contributor

What's the point of keeping these packets if you want to remove them in 1.6 anyway? What is going to change in 1.6?

Contributor

Dezash commented Jul 19, 2018

What's the point of keeping these packets if you want to remove them in 1.6 anyway? What is going to change in 1.6?

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 19, 2018

Member

Backwards compatibility

Member

qaisjp commented Jul 19, 2018

Backwards compatibility

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 19, 2018

Collaborator

There are netcode changes planned for 1.6, which is why some features won't be implemented yet also.

Collaborator

patrikjuvonen commented Jul 19, 2018

There are netcode changes planned for 1.6, which is why some features won't be implemented yet also.

@Dezash Dezash changed the title from Function setPedJetPack to Function setPedWearingJetpack Jul 19, 2018

qaisjp added some commits Jul 19, 2018

@qaisjp qaisjp requested review from qaisjp and Dutchman101 Jul 23, 2018

@qaisjp

The old jetpack functions need to moved to CLuaManager and then I think that's it! Thanks for this PR.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 24, 2018

Contributor

What about lua_classfunction? CLuaManager doesn't contain any, should it be moved there anyway?
If you are going to merge this now, you should probably revert this commit: d89a095 since it makes the upgrade feature replace the function name with that string.
Also, 5705e46 prevents me from starting the resource with the new jetpack functions:

start: Resource 'test' start was requested (Not starting resource test as server has come back from the future)

Is this suppose to be happening on custom builds?

Contributor

Dezash commented Jul 24, 2018

What about lua_classfunction? CLuaManager doesn't contain any, should it be moved there anyway?
If you are going to merge this now, you should probably revert this commit: d89a095 since it makes the upgrade feature replace the function name with that string.
Also, 5705e46 prevents me from starting the resource with the new jetpack functions:

start: Resource 'test' start was requested (Not starting resource test as server has come back from the future)

Is this suppose to be happening on custom builds?

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 24, 2018

Member
Member

qaisjp commented Jul 24, 2018

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jul 24, 2018

Member

Custom builds don't have a fully valid version number (build number is 0 by default), so certain version checks should probably be skipped/modified

Member

ccw808 commented Jul 24, 2018

Custom builds don't have a fully valid version number (build number is 0 by default), so certain version checks should probably be skipped/modified

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 25, 2018

Contributor

@qaisjp from what I tested, setting it to true just outputs a message saying that the function has been removed (even though it's still usable) followed by strNewName. Setting it to false attempts to replace the function name with strNewName.

Contributor

Dezash commented Jul 25, 2018

@qaisjp from what I tested, setting it to true just outputs a message saying that the function has been removed (even though it's still usable) followed by strNewName. Setting it to false attempts to replace the function name with strNewName.

@botder botder added the enhancement label Jul 29, 2018

@qaisjp

qaisjp approved these changes Jul 31, 2018

qaisjp added a commit to multitheftauto/mtasa-resources that referenced this pull request Jul 31, 2018

@qaisjp qaisjp merged commit fe8a534 into multitheftauto:master Jul 31, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 31, 2018

Member

be8c989 is dirty and doesn't actually revert ...

(edit: fixed in 96830bb)

Member

qaisjp commented Jul 31, 2018

be8c989 is dirty and doesn't actually revert ...

(edit: fixed in 96830bb)

qaisjp referenced this pull request Jul 31, 2018

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Aug 7, 2018

@patrikjuvonen patrikjuvonen moved this from In progress to Done in release/v1.5.6 Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment