Skip to content
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 new glitch (don't burn flipped cars) #2369

Merged
merged 13 commits into from
Jul 25, 2023

Conversation

samr46
Copy link
Contributor

@samr46 samr46 commented Sep 22, 2021

Solves #1076

Adds new property "dontburnflippedcars" to functions: setGlitchEnabled, isGlitchEnabled.
Default: false (cars start to burn when upside down) (current behavior)

If set to true then cars will not start to burn when upside down.

setGlitchEnabled("dontburnflippedcars", true)

It doesn't affect any other vehicle health related behavior. The vehicles can be damaged and health can be changed using setElementHealth. Will be useful to make specific vehicles burn upside down if needed.

Test resource: burnflippedcars.zip (there is README.txt inside with some notes and suggestions about testing)
UPDATE 2023-04-09: Current test resource is partially outdated. Function switched to setGlitchEnabled.

@ffsPLASMA
Copy link
Contributor

Out of curiousity, how does the sync work?
For example client 1 has this world property enabled, client 2 has it disabled. The car starts burning depending on who is the syncer?

@LosFaul
Copy link
Contributor

LosFaul commented Sep 22, 2021

Out of curiousity, how does the sync work?
For example client 1 has this world property enabled, client 2 has it disabled. The car starts burning depending on who is the syncer?

good question
from experience burning cars without a syncer don't explode at all

@samr46
Copy link
Contributor Author

samr46 commented Sep 22, 2021

For example client 1 has this world property enabled, client 2 has it disabled. The car starts burning depending on who is the syncer?

Yes, it would start burning for the player that has this feature set to true (and if this player is current syncer then it will start burning for everyone). Only the syncer notifies the server about vehicle health change so it wouldn't look good for players with different options. It's a client side feature (like other world special properties). And several existing world properties can cause visual sync related issues as well if not used for all players.

Normally it should be used for example in onClientResourceStart event. At least, this is intended usage and i don't see any reason someone would like to do it differently.

I also don't see any need to make it sever side with extensive sync handling and essentially vehicle specific. Making it work this way would require hooking several functions with additional logic (completely unnecessary complications).

@Zangomangu
Copy link
Contributor

I also don't see any need to make it sever side with extensive sync handling and essentially vehicle specific. Making it work this way would require hooking several functions with additional logic (completely unnecessary complications).

Agreed.

Would it be difficult to make unoccupied and ped occupied vehicles ignite when flipped?

@samr46
Copy link
Contributor Author

samr46 commented Sep 22, 2021

Would it be difficult to make unoccupied and ped occupied vehicles ignite when flipped?

You mean by patching the game itself or using Lua? Because it could be done using Lua easily and more. This is what i had in mind for this feature:

  • You prevent all vehicles from burning while flipped using this new property;
  • Then if you need specific vehicles to burn while flipped you simply use setElementHealth to set it on fire. Basic timer that checks streamed-in synced by local player vehicles and applies new health gradually till 0 if vehicle rotation suggests that it is upside down.

I use similar approach right now to prevent vehicles from burning flipped (rotation check + setVehicleDamageProof). The downside of this approach is that it makes the vehicle invincible (it could be solved using custom damage system tho). This new property essentially replacing this whole system without any additional changes in my case.

The game could be patched to handle occupied and unoccupied flipped vehicles differently. But i think there is no need to do it since we will have full control over it with Lua anyway.

@Zangomangu
Copy link
Contributor

Yeah I meant patching the game, but your approach makes sense, so I agree there's no need

@patrikjuvonen patrikjuvonen added the enhancement New feature or request label Sep 26, 2021
@StrixG StrixG added this to the Backlog milestone Sep 26, 2021
@TheNormalnij
Copy link
Member

Cool feature. Works for me.

@samr46
Copy link
Contributor Author

samr46 commented Jan 3, 2022

Yeah, it would be nice to merge it. As soon as someone reviews it (should be easy).

@CrosRoad95
Copy link
Contributor

code looks good for me.
I like this feature, especialy because it can help optimize common thing people are doing

@Disinterpreter
Copy link
Member

Disinterpreter commented Nov 19, 2022

We tested this feature, and this works fine for one player, but in situation with several players, it need to be turned to all i suppose.

I can say that it is strange behavior, but setWorldSpecialPropertyEnabled works same with another parameters like snipermoon, so it's OK.

@CrosRoad95
Copy link
Contributor

committee made out of 5 people tested in detail following feature and expressed a positive opinion on its operation
image

@TheNormalnij
Copy link
Member

This feature works only when all players on server have it enabled.

Also there is a race condition. When player download server resources, he can burn vehicles in sync distance.

I think, it should be serverside function. You should send flag bWorldBurnFlippedVehicles in join packet and a packet when you enable this feature. In this case it'll works perfect and i can merge this PR.

@samr46
Copy link
Contributor Author

samr46 commented Nov 19, 2022

This feature works only when all players on server have it enabled.

This is exactly how I saw it used. It was discussed earlier in the conversation.

Also there is a race condition. When player download server resources, he can burn vehicles in sync distance.

This edge case can be easily avoided using download_priority_group in meta.xml (like using some dummy resource without files). But it will not be needed for most servers anyway because normally you need to set this once before player spawns.

I think, it should be serverside function. You should send flag bWorldBurnFlippedVehicles in join packet and a packet when you enable this feature. In this case it'll works perfect and i can merge this PR.

I personally think that it's completely unnecessary. But if this is the only way for it to be merged then maybe someone will be interested in implementing server side function. I'm not gonna be able to do this anytime soon.

@TheNormalnij
Copy link
Member

TheNormalnij commented Nov 19, 2022

This edge case can be easily avoided using download_priority_group in meta.xml (like using some dummy resource without files). But it will not be needed for most servers anyway because normally you need to set this once before player spawns.

Yeah, i know about it, but default camera position can still trigger this issue.
Serverside function will be simple API for server developers w/o additional steps, that developer can miss

@samr46
Copy link
Contributor Author

samr46 commented Jan 23, 2023

Serverside function will be simple API for server developers w/o additional steps, that developer can miss

I guess we could use setGlitchEnabled for this instead of adding new functions. Something like "dontburnflippedcars". Not sure about the name.

@patrikjuvonen
Copy link
Contributor

I think the point about race condition trigger is valid and hence would be necessary to implement a server synchronized setting for this before merging.

@patrikjuvonen patrikjuvonen added the feedback Further information is requested label Apr 8, 2023
@patrikjuvonen patrikjuvonen marked this pull request as draft April 8, 2023 13:46
@samr46 samr46 changed the title Add new world special property (burn flipped cars) Add new glitch (don't burn flipped cars) Apr 8, 2023
@samr46
Copy link
Contributor Author

samr46 commented Apr 8, 2023

Function changed to setGlitchEnabled ("dontburnflippedcars")
Usage: setGlitchEnabled("dontburnflippedcars", true)

@samr46 samr46 marked this pull request as ready for review April 8, 2023 22:55
@botder botder merged commit 21ffd15 into multitheftauto:master Jul 25, 2023
6 checks passed
@botder botder removed the feedback Further information is requested label Jul 25, 2023
@botder botder modified the milestones: Backlog, 1.6.1 Jul 25, 2023
@botder
Copy link
Member

botder commented Jul 25, 2023

@samr46 Please update the wiki page for glitches.

@AlexTMjugador
Copy link
Member

AlexTMjugador commented Jul 25, 2023

I like the idea of making this feature toggleable through server functions, but shoehorning it into setGlitchEnabled feels a bit odd to me, since it's not a singleplayer glitch fix. It's hard to discover that setGlitchEnabled is capable of toggling this by skimming through the wiki function lists.

How feasible would it be to move it to setWorldSpecialPropertyEnabled and make that function shared? The latter is a useful quality of life improvement, as several typical uses of setWorldSpecialPropertyEnabled change properties for all clients anyway.

@botder
Copy link
Member

botder commented Jul 25, 2023

@AlexTMjugador That sounds right. It's time to add setWorldSpecialPropertyEnabled to the server and move 'dontburnflippedcars' there.

@AlexTMjugador
Copy link
Member

Alright! @samr46, would you like to file a separate PR for that? I wish I had reviewed this before the merge, but I didn't notice the change to use setGlitchEnabled until now, sorry 😅

@botder
Copy link
Member

botder commented Jul 25, 2023

Please don't bother, I am already working on that.

@samr46
Copy link
Contributor Author

samr46 commented Jul 25, 2023

Alright! @samr46, would you like to file a separate PR for that? I wish I had reviewed this before the merge, but I didn't notice the change to use setGlitchEnabled until now, sorry 😅

Just created new branch 😂
But it will be faster for @botder to do it and merge right away. I will edit wiki again to reflect this afterwards.

botder added a commit to botder/mtasa-blue that referenced this pull request Jul 25, 2023
…ultitheftauto#2369)"

This reverts commit 21ffd15.

The changes will re-introduced in a short while.
@botder
Copy link
Member

botder commented Aug 14, 2023

@samr46 Sorry for this, but can you do it instead?

@samr46
Copy link
Contributor Author

samr46 commented Aug 16, 2023

Sorry for this, but can you do it instead?

Yes, I will send new PR ~ tomorrow.

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

Successfully merging this pull request may close these issues.