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

Enhance table.Copy #2036

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

Enhance table.Copy #2036

wants to merge 8 commits into from

Conversation

Zaurzo
Copy link
Contributor

@Zaurzo Zaurzo commented Dec 9, 2023

Changes

Added Arguments
copyVecAngMx - Will make the function copy Vector, Angle, and VMatrix objects.
noMeta- Will not set the metatable of the given table to the copy result, if there is one.

Additional Changes
The function will yield an error if the first argument isn't a table.
The function now only sets the metatable of the given table to the copy result after everything has been copied.
Updated the command description in the comment above it.

There is some changes I want to discuss in this PR, however:

  1. Is making the function error a bad idea? Will it break some addons?

  2. Will making the function set the metatable to the copy after it's done copying cause issues?

  • I made this change because a table I was trying to copy had its own __newindex behavior, and since the original function set the metatable before it added all of the values, it caused some problems.
  1. Should the copyVecAngMx argument even exist, or should I make it always copy Vector, Angle, and VMatrix objects?

  2. Is the backwards compatibility for the lookup_table argument even necessary?

I am looking forward to reading the thoughts on this. Thanks

Added arguments fullCopy and noMeta, made it error if the first argument isn't a table, improved the comment, and a few other changes.

fullCopy will make the function copy Vector and Angle objects
noMeta will ignore the metatable of the given table
- Edge case for colors not being given the Color metatable is noMeta is on
- Fix fullCopy ignoring values that aren't Vectors or Angles
@robotboy655 robotboy655 added the Enhancement The pull request enhances current functionality. label Dec 11, 2023
@Denneisk
Copy link
Contributor

Would suggest changing fullCopy to copyUserdata or something more precise to indicate that it's specifically for vectors/angles; might be confused with shallow/deep copying otherwise.

I believe sub-tables shouldn't be effected by noMeta, since they might be used for objects.

@Zaurzo
Copy link
Contributor Author

Zaurzo commented Dec 11, 2023

Would suggest changing fullCopy to copyUserdata or something more precise to indicate that it's specifically for vectors/angles; might be confused with shallow/deep copying otherwise.

I believe sub-tables shouldn't be effected by noMeta, since they might be used for objects.

Changing fullCopy to copyUserdata might better, yeah, however this implies it copies all userdata which it doesn't.

Yeah, changing noMeta to not affect sub-tables seems better. I was already aware of this problem of it not setting the metatables for custom objects like ones from util.Timer()

@Denneisk
Copy link
Contributor

Changing fullCopy to copyUserdata might better, yeah, however this implies is copies all userdata which it doesn't.

Right, then copyVecAng or something.

@Zaurzo
Copy link
Contributor Author

Zaurzo commented Dec 11, 2023

Changing fullCopy to copyUserdata might better, yeah, however this implies is copies all userdata which it doesn't.

Right, then copyVecAng or something.

Gotcha.

@Kefta
Copy link
Contributor

Kefta commented Dec 12, 2023

Why copy vectors and angles but not matrices and colors?

@Zaurzo
Copy link
Contributor Author

Zaurzo commented Dec 12, 2023

Why copy vectors and angles but not matrices and colors?

Colors: Colors are tables, it already copies them.
Matrices: I forgot about them

@Zaurzo
Copy link
Contributor Author

Zaurzo commented Dec 12, 2023

Added VMatrix support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement The pull request enhances current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants