Fix for the GetParentTypeDisallowingMultipleInclusion / GetRequiredComponents #1
Conversation
…ed by using AddComponent and instantiating prefabs at run-time introduced years ago by simply caching data.
|
Preferably al the methods in this class would be cached. I'd suggest making another One thing that might complicate this is the inherit argument for GetCustomAttributes, sometimes you do want the inherited attributes and other times you dont. Maybe it could create a cache of all attributes of a type, and return a subsection of them based on the parameters. This is an example of just pressing Play in the editor in our fairly large project. Its too bad that there doesnt seem to be a way to trick Unity into using our own compiled dlls, i'd love to see if making them all returning the inherited attributes changes behaviour much. I really wouldnt dare to commit something like this without being able to test it properly. |
|
You do realise that by even writing the code for this PR, you've violated the "Reference Only" license that this source code is provided under, right? |
|
@TJHeuvel your can edit the UnityEngine.dll and recompile it with this fix yourself. I've been doing this for months (http://www.tasharen.com/forum/index.php?topic=15504.0). It's just not convenient, to say the least. This is Unity's bug to fix, and us hacking around it for them is not ideal. |
|
@ArenMook I know, i told you months ago. What i am saying is lets not stop at a bit of a hacky fix, but lets go through the codebase and fix it properly. @hach-que I guess you are right, and they probably wont accept PR's. Lets wait for the official announcement! |
|
@ArenMook completely agree that this is something we should fix on our side. We can't accept PRs via this repository though (changing the code is against the license, strictly speaking! plus various issues like rights to code etc.). Maybe at some point we'll have a smoother approach for that kind of collaboration, but not today just yet. We would have disabled PRs on the repository, if github had that functionality. You're all welcome to submit suggestions or ideas for improvements via bug reporter and other channels. |
|
https://fogbugz.unity3d.com/default.asp?746364_pjnmdhk7c9imgdsk |
|
@aras-p yup, but as you know this bug was reported years ago and nothing was done about it. Me mentioning it on Twitter months ago and even talking to a dev in Unity didn't fix it either. So this is my fix, submitted against your codebase, and all you have to do on your end is push a button. :) As for rights to the code -- Unity owns it 100%. Pull requests included. It's your codebase. It's not tricky at all in my mind. |
|
@ArenMook It's not tricky for you, and not tricky for me, but the kick is -- neither of us are lawyers. Modifying the code is against the license, hence we can't accept PRs. Full stop. I can't "just push a button". Would I like the world where this problem does not exist? Yes I would. Sadly it's not the world we live in :( |
|
Case 746364 was closed as "put on the internal roadmap of the team", I'll poke them with what's up with it. |
|
@aras-p then don't accept the pull request. Copy-paste my code into your Visual Studio, check it in as your own work. I'm cool with that. All I want is the bug that users have complained about for years fixed. That's all. :) |
|
Sure, I'm talking with folks right now. I still can't "just copy paste" this, due to legal reasons. Anyway, everyone agrees this should be fixed :) |
|
Awesome, thanks a bunch. :) |
|
@ArenMook "As for rights to the code -- Unity owns it 100%. " In most european countries there is no legal way to entirely surrender your rights to your creation. |
|
@aras-p can you ask the lawyers whether a Contributor Contract or similar would solve the issue? Google does the same to allow people to contribute to their repos. It limits liability and puts the burden on the contributors in case any malicious or copyrighted code is contributed. |
|
Yes, I know the motions we'd have to go through to maybe someday have a better workflow of submitting the patches. All I'm saying is explaining why we aren't accepting PRs today. Maybe someday we will, who knows! But as it is right now, my only point is that I can't "just click a button", ok :) |
|
This was a fun read on Sunday morning :) great work and hopefully it gets ingested. |
|
The community is eager to help Unity help itself. The sooner, the better. |
|
About legality, there are companies that ask us to sign a NDA that states, that every commit we do is company's IP and not Original Poster's IP. (just broke that NDA :D), though this is very, very common practice. E,g, transfer ownership. Sadly, lawyers will try to defeat everyone with just words and that is a business... there are many companies that write something in legalese but then explain their intents in verbs and does not sue minor breachers. Having this reference source in a form that can be recompiled and replaceable/runable means that we have something to tinker with, and suggest fixes like ArenMook did. Google work like this, also - they just want you to sign some contribution license that back in the days I didn't read completely because to me it was clear. I do something for somebody, I abandom my rights on it. So a good flow will be when people decide to post a PR here despice the license disallowing them to tinker (local regulations in my jurisdiction render this clause invalid after all :D), UT immediately REJECT the PR but still consider it internally. After all we, when post a PR, are doing this to help all else unity developers, not just ourselves. Using this PR system we can actually provide some clues to long-years-not-fixed issues or true WNFs. UT can of course rage at people posting PRs here even ban/terminate their licenses, but we all know, as a devs, why this is not of theirs interest :) And @aras-p, as an UT insider, I hope that you do analyze the UT competitors :) Epic have very well esablished workflow - you just need to register, pass through the NDA and check it by yourself :) |
|
Wow how Fast !!! |
|
@TJHeuvel I suggest removing the access code from /default.asp?746364_accesscode as it allows anyone with the link to view all your submissions, messages etc : ) |
|
Consider going open source. It will greatly improve overall engine performance and quality. |
|
@aras-p I understand Unity not being able to accept our pull requests per legal reasons, but I think it could be beneficial to let people use the issue tracker section to work together in some ways. For example, we can use it such as how @ArenMook did to fix bugs with each other between official releases. In turn, Unity Devs could see how we are fixing the problems and skip over some of the trial-and-error debugging and code tracing to get right to the issues, which would let them put more work into each release and fix more bugs/add more features. Just my thoughts anyways :) |
|
I was surprised at how quickly someone already made a PR lol. Hopefully Unity's lawyers see how eager people are to help contribute to the codebase and figures out a way to legally let us submit patches and fixes for things like this and other bugs that have been plaguing Unity for the longest time. Especially since Unreal Engine has made ALL of their source code available for viewing and editing. Maybe Unity can take some cues from Unreal and even make their C++ side open source 😄 But this is already a huge leap and I commend Unity for going down this path! 🎉 |
|
There's always UE4... |
|
Now Unity has 2 options:
Hopefully Unity will figure out a way to make this work with their business and allow us to make the multiple bug fixes we have been waiting for way too long already. |
|
If developers see the solutions other programmers PR and then apply them on their own, because the other programmers have not been cleared by legal then Unity can be lawyered for stealing ideas |
There was a problem hiding this comment.
This is not a thread-safe solution.
Use [ThreadStatic] and move the field initialization to the method like that:
[ThreadStatic] static Dictionary<Type, Type> m_BaseTypeLookup;
...
static Type GetParentTypeDisallowingMultipleInclusion(Type type)
{
if (m_BaseTypeLookup == null) m_BaseTypeLookup = new Dictionary<Type, Type>();
...
Or move it to a generic type static class YourCache<T> and store BaseType and RequiredCompLookup as Type and Type[]. Use MakeGenericType to resolve that cache and access to the fields (i'm not sure about perfomance).
https://stackoverflow.com/questions/44034880/c-sharp-resolving-a-type-as-generic-type
| @@ -26,11 +26,17 @@ namespace UnityEngine | |||
| { | |||
| class AttributeHelperEngine | |||
| { | |||
| [System.NonSerialized] static Dictionary<Type, Type> mBaseTypeLookup = new Dictionary<Type, Type>(); | |||
There was a problem hiding this comment.
Unity field member prefix coding style is with m_.
|
@aronmook under what license did you publish the fix? I would suggest http://www.wtfpl.net/txt/copying/ |
|
how about add a license to your fix？ for example, WTFPL |
|
Okey enough bullshit. Unity (@aras-p ), How to do this the right way, forward to the CEO.
This taken into account there is NO LEGAL ISSUE WHAT SO FUCKING EVER. For the CLA you could even use cla-assistant.io but if Unity does not want it managed by an external party you guys could even send them out as needed on good damn paper. The reason companies managing open source projects use CLA's is (as exampled by @petersvp ) to transfer full ownership to prevent future licence issues. Sorry, but the excuses posted here have close to zero actual legal ground. @ArenMook If you want to make it easy for their retard lawyer, you can write a formal signed transfer of copyright ownership for them. TL:DR |
|
Seriously, guys. (I should have left this conversation, right?) Point t0: Unity does not have any C# source published. Is t1 better state than t0? I'd like to think it is! Would it be better to have a hypothetical t2 where there's a more permissive license and a PR process? Sure. Does it mean doing t1 in the meantime is useless? I don't think so. Reading the comments here, it seems that everyone's "lol lame, ur guise r totally brainded". THANKS. Next time I should know to not even bother trying and won't do any "t1" like steps for anything. You win, internet. |
Or for the time being, 3. Fix the underlying issue on our own without directly using this patch. This is happening as we speak. |
|
Maybe straight up mention in your readme at the very top that at this point in time you won't be accepting any PR. People might not read what's in their face, but at least it's something to point at. |
|
Usually, The normal flow of stuff is Aras::t0 -> Aras::t1 -> hypothetical Aras::t2. Though, If you guys don't stop insulting Aras's collegues [e.g. the lawyers], we risk being reverted back to state t0. So, if I have to be vulgar, just "Shut Up" and accept the situation whatever it is because nothing can stop them to hit the Delete key because of you acting childish [where you ~= the braindeads], right? @Ornias1993 you are not funny, really. You are too vulgar so anything you stated, even if it makes a lot of sense, it's automatically devalued because of your vulgarity. I don't want to feel "fucked" in a game development forum. We have porn-sites for that. Or girl/boyfriends. UT are doing something, even small, as reference, for us. That is why I respect them. Yes, my position is also that it should progress further and I believe it will, though we don't need such drama right now. Looks like they are already working on the @ArenMook bug that we all hated. |
|
@aras-p I was super skeptical because as mentioned before this is a years old issue, but just the fact that you guys are doing the best you can with the current legal limitations is a good sign! Your answer gives me hope that the way issues are fixed will only improve I'm sure it will benefit everybody if Unity keep moving towards being more open But anyway, right now I believe you guys are trying the best, and that really makes all the difference for everybody working with the engine. keep it up! |
|
Alright, with the issue being fixed in Unity, the whole point of this PR is, in essence complete. Discussions on legal matters aren't really why I made the PR, so let's do that elsewhere. As such I am going to close this. Thank you everyone for your feedback, and @aras-p for listening. :) |
|
Merged #1.
|
|
@petersvp It was not really meant to be funny and not even vulgar. Besides, if you rater put your "value" in something that is quite obviously complete bullshit because the "tone" is nicer. Well not everyone lives in fairyland where everyone is nice and professional. |
|
I wanted to let you know that this issue has now been fixed in Unity 2018.3.a9 by caching the attribute data per class in native code. Release notes |
|
Nice, thanks! |
Fix for the massive amount of GC memory allocations and slowdown caused by using AddComponent and instantiating prefabs at run-time introduced years ago by simply caching data. Expected result of the fix: https://pbs.twimg.com/media/DOUnrDDXkAAcexq.jpg