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

0009891: Add support for more map attributes #263

Merged
merged 35 commits into from Apr 1, 2020
Merged

0009891: Add support for more map attributes #263

merged 35 commits into from Apr 1, 2020

Conversation

patrikjuvonen
Copy link
Contributor

@patrikjuvonen patrikjuvonen commented Jul 26, 2018

Mantis Bug Tracker issue:
9891
fixes #454

Summary:

  • Add visibleDistance and size attributes to <blip>
  • Add headless and walkingStyle attributes to <ped>
  • Make "all" option case-insensitive for upgrades attribute on <vehicle>
  • Add headLightColor, taxiLightOn, engineOn, lightsOn, damageProof, explodableFuelTank, toggleRespawn, respawnDelay, respawnPosX, respawnPosY, respawnPosZ, respawnRotX, respawnRotY, respawnRotZ attributes to <vehicle>
  • Make code style more consistent
  • mtasa-resources edf + editor update issue + PR open here: edf: add missing map attributes mtasa-resources#115

Test map file:
test_263_v2.txt

@patrikjuvonen patrikjuvonen added the enhancement New feature or request label Jul 26, 2018
@patrikjuvonen patrikjuvonen changed the title Issue 9891 0009891: add support for more map attributes + hex & rgb colors Jul 26, 2018
@patrikjuvonen patrikjuvonen changed the title 0009891: add support for more map attributes + hex & rgb colors 0009891: add support for more map attributes + better support for hex & rgba colors Jul 26, 2018
@patrikjuvonen patrikjuvonen changed the title 0009891: add support for more map attributes + better support for hex & rgba colors WIP 0009891: add support for more map attributes + better support for hex & rgba colors Jul 26, 2018
@patrikjuvonen patrikjuvonen added in progress bug Something isn't working labels Jul 26, 2018
@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Jul 26, 2018
@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Jul 26, 2018
@patrikjuvonen patrikjuvonen changed the title WIP 0009891: add support for more map attributes + better support for hex & rgba colors 0009891: add support for more map attributes + better support for hex & rgba colors Jul 26, 2018
@patrikjuvonen patrikjuvonen changed the title 0009891: add support for more map attributes + better support for hex & rgba colors WIP 0009891: add support for more map attributes + better support for hex & rgba colors Jul 30, 2018
@patrikjuvonen patrikjuvonen changed the title WIP 0009891: add support for more map attributes + better support for hex & rgba colors 0009891: add support for more map attributes + better support for hex & rgba colors Jul 31, 2018
@patrikjuvonen patrikjuvonen changed the title WIP: 0009891: Add support for more map attributes 0009891: Add support for more map attributes Sep 6, 2019
@qaisjp
Copy link
Contributor

qaisjp commented Sep 6, 2019

  • Introduce new forceColor attribute (default: false), which can be used to toggle whether color attribute being 0,0,0 will be read as forcefully black or random (default color will be overridden by force). Added for backwards compatibility.

I was thinking:

  • the default value of randomColor would be black, which is the old functionality
  • randomColor=true would force a random color
  • randomColor=false would allow you to use the black color

But then it isn't obvious what randomColor does by default (I would expect it to be false if I was reading a .map file).

And my worry with forceColor is that it isn't very semantic & has the same issue as above (could someone know what forceColor=false does without reading the docs?)

Perhaps we could do a census (ask racing servers to run a script for us and tell us the output), and if results say that color=0,0,0 is rarely used, we can just choose to break backwards compatibility?

It's up to you really, that sort of census sounds like too much work it would drag this PR on for too long

@patrikjuvonen
Copy link
Contributor Author

patrikjuvonen commented Sep 7, 2019

I think the problem here is that by introducing randomColor attribute I'm expecting to get a random color doesn't matter what I input into the color attribute, at least in my opinion. This behavior would break existing maps.

Because it's actually quite "ambiguous" what the default behavior is, by introducing a forceColor attribute, we will make it clear that whatever our vehicle class does now or in the future by default will be overridden by the color attribute.

So whichever way we go, people would have to read up on Wiki what it does.

We could however name it __backwardsCompatibleColorBehavior, __useOldColorBehavior (default true) or something other specifically dirty so it marks that this isn't good behavior and may be changed in the future to only work with the new style which is that 0,0,0 will result in black color, not random color.

I do however believe we should introduce a new randomColor attribute that will generate a random color even if the current default vehicle class behavior was refactored and random color wouldn't happen by default. We could even make it so that if no color attribute exists on the node, it will generate a random color, however I think this behavior would break more maps.

And meanwhile, we could inquire race server owners whether they have any color="0, 0, 0" strings in any of their map files, then replacing all of these occurrences by randomColor="true", if their intended behavior was indeed to get a random color, and not black color.

@Woovie
Copy link
Contributor

Woovie commented Jan 17, 2020

I contacted AfuSensi on Discord, who owns/operates/works on Mr. Green's

Search "<vehicle.*color="0,0,0" (1485 hits in 167 files)

Search "<vehicle.*color="([1-255]{1,3}),\1,\1" (82 hits in 33 files)

Around 2500 maps total, and this is the total amount of returns.

@patrikjuvonen
Copy link
Contributor Author

Thanks @Woovie, I think we should separate the vehicle color fix as its own PR and issue so we can discuss that matter in a different thread to see what would be the best solution. I do believe something has to be done since the current default behavior is not expected behavior in my opinion.

@Woovie
Copy link
Contributor

Woovie commented Feb 6, 2020

So we should revert 274d9b3 and I guess it's ready for merge? @qaisjp what do you think?

@qaisjp
Copy link
Contributor

qaisjp commented Feb 6, 2020

So we should revert 274d9b3 and I guess it's ready for merge? @qaisjp what do you think?

sounds fine

@qaisjp qaisjp removed their assignment Feb 6, 2020
@qaisjp qaisjp removed their request for review February 6, 2020 19:35
This partially reverts commit 00ad397.

Removed new bForced argument from SetRGBColors and SetPaletteColors
functions as it is related to another out of scope issue.
@patrikjuvonen
Copy link
Contributor Author

patrikjuvonen commented Feb 22, 2020

I have now removed the forced color attribute from the PR. Making a new issue about it now, and also of the broader color syntax seen earlier in this PR.

Black RGB color attribute issue created at #1257
Hex and RGBA color attribute support issue created at #1258

@qaisjp
Copy link
Contributor

qaisjp commented Mar 22, 2020

go for merge, lgtm, :shipit:, @patrikjuvonen

@qaisjp qaisjp modified the milestones: 1.6, Backlog Mar 23, 2020
@patrikjuvonen patrikjuvonen self-assigned this Apr 1, 2020
@patrikjuvonen patrikjuvonen merged commit ffa08f9 into multitheftauto:master Apr 1, 2020
@patrikjuvonen patrikjuvonen deleted the issue-9891 branch April 1, 2020 14:51
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.

Add missing map attributes
4 participants