Closed StephenHodgson closed 7 years ago
I think there are multiple questions here:
Should we drop support for existing SendMessage systems? Breaks old functionality to remove so I'd advise against.
I can see this being the only real sticking point for some people. But the benefit would be the use of good design choices/practices that would help them see improvements in their application performance.
I'd advise to mark the old functionality as obsolete to give people time to move over to using the new.
Notice how OP could explain the send message with the code GameObject.SendMessage("Method"); but had to link to a video to explain how delegates work. I think its not easier.
Making code more complicated and thus less accessable is not worth it for a few nanoseconds for an event that uses tens of thousands of times more for the recognizers. This would be a clear example of premature optimization.
Notice how @izatar uses opinion rather than facts to back up their point. This is not politics. The link used was to illustrate a clear example of how to implement this properly, not explain how they work.
Did you have anything constructive to add to your opinion? Maybe references, code snippets, or test projects you could contribute to state your case and further our discussion?
This would be a clear example of premature optimization.
Yes, premature optimization can be bad, but we're talking about a 85.5x speed improvement. Remember, the HoloLens is a mobile device, with a limited amount of performance and overhead (It's recommended that we need to ALWAYS keep 60 fps). It's safe to say that something like this should have been discussed and talked about about during preproduction.
We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%.
Not to mention, while working at Microsoft last spring on a HoloLens project, we made the decision to design for performance and explicitly not use this design pattern and ensure we followed rules, like for example: how and when we should be allocating memory.
Designing for a particular fitness has tremendous impact on the function and form of the software
In fact I'm surprised to see that many of those same rules are not enforced here.
Making code more complicated and thus less accessible
Actually this does exactly the opposite. If you look at the pull request, there's a reduced number of lines of code effectively reduces complexity. Also, replacing a string with a symbol drastically improves the accessibility and ability to debug. Debugging in VS with SendMessage is nearly impossible unless you know exactly where to look for your trouble spots and place a break point. What about those times where you're unsure, and stepping through the code is your only option to finding the location of your bug?
Why don't you take the time to make the pull, and read the code, and write a constructive review.
My reference to "the OP" was not intended as a personal attack. May I suggest that you could spend as much effort finding the weaknesses in your own argument as you did in mine?
I didn't think it was a personal attack by any means. I welcome criticism and critique. That's the only way for professionals like us to learn, grow, and hone our abilities. Sorry, I spent waaaaay too much time on the debate team.
Hi there,
First thanks and kudos for doing this "massive" work.. I've been thinking about doing something similar but never found the time. I'll go through your PR asap but the only concern I have is on backwards compatibility issue. Even though this design is better on many levels, right now getting the latest version of the HTK means ppl would have to update their code and that's IMO is a no go (your update is a major change and the fact there's no versioning system or release notes in place here doesn't help of course..) But basically ppl would get the latest version and all the HL input wouldn't work anymore. Also there's a lot of people who don't really care about performance, and arguably the SendMessage approach is ok for "simple" cases. So my recommendation would be to keep the SendMessage API (and set it on per default) and add a new mode on the GazeManager etc.. (that was my plan but you were faster than me :-) Any thoughts ?
@stbertou I totally agree, and have already suggested keeping the old SendMessage stuff around, and marking it as obsolete to give people time to transition over, although I still need to address this in the pull request (Partly because I'd like to see this reviewed over a week or two period to ensure others have a chance to voice suggestions).
Yes, I too found that a lot of the time I was just rewriting the what what already in the HTK, but was frustrated by this because I wanted to stay open to future updates. That was my main driving factor behind these changes.
Thank you everyone for the hard work. We all should be respectful and open to each other's thoughts and comments. As said earlier, we all are learning and growing together.
Many people look at the HoloToolkit as a learning or starting point as well for Unity and HoloLens interaction. SendMessage is not ideal for reasons stated above but many developers find it super simple to understand and easy to use. Hence it exists and the reason it was used to teach concepts.
I am totally onboard for a transition plan. Such large breaking changes obviously won't work. We need a better story for breaking changes and I will try to come up with a plan for that.
Thanks again! Will review the PR shortly and sorry for the delay.
Path forward for this feature:
Here's what I recommend we do for such features as documented in Contributing.md:
Questions/concerns, please let me know.
@HodgsonSDAS a new branch called Feature-SendMessageRemoval has been created. Please feel free to use it for adding this feature. Also please close without merge the existing PR when ready. Thanks everyone!
@darax I went ahead and implemented the Interface you were talking about instead of using the virtual methods for our Interactable class. I see a lot of potential in that. I think when it comes down to it, reducing code complexity and increasing debug ability are the real winners here.
Although, one drawback is that the method for the Interface will also be public, so anyone outside our class could potentially call OnTap (or whatever else we're implementing).
From my experence and from what others say this quote from the Perf Recomendations may or may not be correct:
Avoid interfaces and virtual methods in hot code such as inner loops. Interfaces in particular are much slower than a direct call. Virtual functions are not as bad, but still significantly slower than a direct call.
But seeing as we wouldn't be using it in hot code, I don't really see a difference besides implementation and ease of use.
We could potentially use both. For most objects we might want to use the Interface, but in some cases, maybe we only want to subscribe one or a few listeners to our tapped event. I think having the ability to subscribe to the GestureManager to get the tapped object callbacks would be useful.
Check out ChangeColorOnSelect.cs
and ChangeColorOnFocus.cs
for examples of usage.
I don't disagree with this change, but that article doesn't really support removing SendMessage for performance reasons the way we use it. About SendMessage:
That said, each function type was called 100 million times, so it’s not so slow that you should never use it. Each call was only 6/10,000 of a millisecond, so feel free to make a few of these calls without worrying that it’ll ruin your app’s performance.
I'm pretty sure we call (most of? all of?) these one frame at a time. If we had a couple hundred every Update, maybe it'd be noticeable.
@keveleigh I think it's more about readability and debug ability more than anything, I just wanted to share that's all.
@HodgsonSDAS Ahh yep, definitely agreed with you there! It is interesting to see the data as well. Thanks for the share!
Pull Request https://github.com/Microsoft/HoloToolkit-Unity/pull/244
Many experienced Unity developers would probably tell you not use GameObject.SendMessage("String"); because it's slow (under the hood it uses reflection), hard to debug, and it lacks direct symbol references in the editor which makes it prone to spelling errors because it's a string and not a symbol. This can also make it difficult to find everywhere the method referenced can be called and could potentially increase the amount of spaghetti code. It may be worth while to see this updated to utilize a delegate/Event call instead or implement an Interface.
Another good link that explains the difficulty with using SendMessage
This will make the code easier to follow in the editor, easier to debug, and will increase performance.
Some arguments for using delegate events vs SendMessage:
Another major benefit of using Delegate/Event is type checking. A delegate specifies specific types that must be passed as parameters, while SendMessage just accepts an object.
A good argument for using Interfaces vs delegate events:
Although, one drawback to the Interface Implementation is that the method for the Interface will also be public, so anyone outside our class could potentially call OnTap (or whatever else we're implementing).