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

New Lua constants (#3022) #3039

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

TracerDS
Copy link
Contributor

@TracerDS TracerDS commented May 24, 2023

@tederis
Copy link
Member

tederis commented May 24, 2023

I think that the length of these constants makes them cumbersome and difficult to use. I doubt that long constructions like constants.VehicleLightOverride.Disable can be usefull. I would suggest to make them shorter.

@TracerDS TracerDS marked this pull request as ready for review May 24, 2023 13:32
@tederis
Copy link
Member

tederis commented May 24, 2023

In addition, I would suggest to make the table read-only.

Copy link
Contributor

@CrosRoad95 CrosRoad95 left a comment

Choose a reason for hiding this comment

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

Fine for me, just maybe other people may have concern about names, but apart of that it is good to go

@lopezloo lopezloo added the enhancement New feature or request label May 24, 2023
@patrikjuvonen patrikjuvonen changed the title New Lua constants - #3022 Fix #3022: New Lua constants May 25, 2023
Copy link
Contributor

@Pirulax Pirulax left a comment

Choose a reason for hiding this comment

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

Also, whats the point of not directly assigning the values to constants.* but instead making a local var first?
If there's no good reason, then we should just assign it directly [this also reduces the chance of interfering with existing scripts]

Also, there's a chance that some scripts might break if these constants are loaded into every script, we should have a meta.xml toggle for it, that toggles this feature per-resource [as theres one LuaVM per resource]

Also... I'm not sure if constants is the solution.
Perhaps we should just push them into the global namespace? [Like KeyStates.Up rather than constants.KeyStates.up.]
[I'm sure some people would prefer the one over the other]

And, as @tederis noted, making the tables read only might be useful [though Lua is somewhat limited in that regard]

@Pirulax
Copy link
Contributor

Pirulax commented Jan 6, 2024

Also something I just realized, these constants are pretty useless on their own.
There should be a way to convert to/from name/value.
Given:

local MyEnum = {
    First = 1
}

MyEnum['First'] should return 1. MyEnuim(1) should return First
This is similar to how Python work (Though Python returns special EnumValue objects that we can't afford in terms of performance, so we're going with the simpler solution here).

Tables are now read only;
Better value handling
@TracerDS
Copy link
Contributor Author

I think that the length of these constants makes them cumbersome and difficult to use. I doubt that long constructions like constants.VehicleLightOverride.Disable can be usefull. I would suggest to make them shorter.

What would we rename them to? 🤔

Also something I just realized, these constants are pretty useless on their own. There should be a way to convert to/from name/value. Given:

local MyEnum = {
    First = 1
}

MyEnum['First'] should return 1. MyEnuim(1) should return First This is similar to how Python work (Though Python returns special EnumValue objects that we can't afford in terms of performance, so we're going with the simpler solution here).

Implemented. Thanks for the suggestion ❤

@TracerDS TracerDS changed the title Fix #3022: New Lua constants New Lua constants (#3022) Jun 1, 2024
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.

7 participants