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

Restore vector type check #14663

Merged
merged 1 commit into from
May 20, 2024
Merged

Restore vector type check #14663

merged 1 commit into from
May 20, 2024

Conversation

OgelGames
Copy link
Contributor

Restores the type check for vectors that was lost in b38ffde.

Example error messages, without and with the type check:

ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'test' in callback ScriptApiEnv::environment_Step(): ...\builtin\common\vector.lua:380: attempt to index local 'v' (a nil value)
ERROR[Main]: stack traceback:
ERROR[Main]:       ...\builtin\common\vector.lua:380: in function <...\builtin\common\vector.lua:379>
ERROR[Main]:       [C]: in function 'set_pos'
ERROR[Main]:       ...\mods\test\init.lua:3: in function 'func'
ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'test' in callback ScriptApiEnv::environment_Step(): "Invalid vector (expected table got nil)."
ERROR[Main]: stack traceback:
ERROR[Main]:       [C]: in function 'set_pos'
ERROR[Main]:       ...\mods\test\init.lua:3: in function 'func'

@sfan5
Copy link
Member

sfan5 commented May 15, 2024

I think we established that table operations are faster from Lua than from C++, so this check will have been intentionally left off.

@OgelGames
Copy link
Contributor Author

But it only checks if the vector is a table, there's are no table operations.

Even if it was intentional I think it should be added back, because it makes the error message clearer.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

I guess.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

LGTM

@sfan5 sfan5 merged commit 8972e82 into minetest:master May 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants