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

I get failure of TryGetValue in PushObject results in wrong data being passed back #73

Closed
LucienMP opened this issue Mar 1, 2014 · 6 comments

Comments

@LucienMP
Copy link

@LucienMP LucienMP commented Mar 1, 2014

When I have this running under Unity (Mono 2.0) I am catching the following problem ONLY on specific data types. Can anyone confirm, or at least explain the issue to me - I am going to assume a broken VM with broken Equals() implementation.

C# Side
struct Top { struct sub y; }
struct sub { struct sub2 z; }
struct sub2 { int someval; }

LUA Side:
-- x=new Top() from C#
print( "Z is type:" .. tostring( x.y.z:GetType() ) .. " should be sub2" )

Result:
Z is type: sub should be sub2

Any attempt at accessing past x.y results in getting back "y" from LUA.
The result seems to be due to TryGetValue's internal "Equals" incorrectly identifying two types as the same. Works fine in MSVC.NET 3.5/4+

Here is the code I inserted to find the issue, and it does hit the debug print and when I check types in LUA I get the message above.

======== SOURCE OF ERROR IN CODE:
ObjectTranslator.cs : PushObject

internal void PushObject (LuaState luaState, object o, string metatable)
...
bool found = objectsBackMap.TryGetValue(o, out index);
if ( found && o != objects[index]) // LMP - Added as extra catch test
Debug.Print("\tAlready found it @ index {0}, types DONT MATCH", index);
...
// So now index is wrong, its going to retrieve the wrong type

@MikeSchurman
Copy link
Contributor

@MikeSchurman MikeSchurman commented Apr 14, 2014

I'm seeing a potentially similar thing, where 2 valuetypes that hash to the same number share an object.

In Lua:
v1 = Vector3(0,0,0)
v2 = Vector3(0,0,0) -- because these vectors are the same value, they share the same object(error)
v1.x = 1
-- v2.x will now equal 1, as v1 and v2 share the same object on the c# side, because it uses objectsBackMap dictionary

@viniciusjarina
Copy link
Member

@viniciusjarina viniciusjarina commented Apr 28, 2014

Hey guys..

I wrote a simple test here using the code from @LucienMP and is working here 😢

Can you guys write a unit test and create a PR ? I will be happy to fix this issue.

Thank you.

@MikeSchurman
Copy link
Contributor

@MikeSchurman MikeSchurman commented Apr 28, 2014

I plan to try and write a unit test, for my case at least. I just need to familiarize myself with your project structure.

@MikeSchurman
Copy link
Contributor

@MikeSchurman MikeSchurman commented May 13, 2014

I finally got that failing unit test in a PR for you. I'm not sure if the fix will be easy. If it is giving you a lot of trouble let me know and I'll think about a work-around. One workaround would be to ensure to only use C# structs in lua that don't hash to the same value (ie. override GetHashCode to use a more class-like hashing function).

I have to say my understanding of hashing/struct/class/equality in C# is not 100%

@thomasfn
Copy link

@thomasfn thomasfn commented May 16, 2014

This caused huge issues in Oxide (https://github.com/RustOxide/Oxide, http://rustoxide.com). LucienMP's fix (implemented in LucienMP@866b842) has stopped the described problem from occuring and has been very widely stress tested. I'm a bit fuzzy on the details now as it's been a while since I looked at this problem, but if the solution committed above can be made less hacky that would be great.

Just to clarify, the issue lies with the fact we're using a very old version of mono (Unity3D's current version) that has a bad implementation of something, it's been fixed in newer versions of mono).

@viniciusjarina
Copy link
Member

@viniciusjarina viniciusjarina commented Jun 21, 2014

Fixed 81d6d81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants