Replace GameObject.SendMessage("Method");

[StephenHodgson]

Pull Request #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

Emblematic example of these bad choices is the communication system built around SendMessage and BroadcastMessage. If you use them, you should just stop already!

In fact SendMessage and BroadcastMessage are so wrong mostly because they heavily rely on reflection to find the functions to call. This of course has a given impact on performance, but performance is not the issue here, the issue is instead related to the quality of the resulting code.

What can be so bad about it? First (and foremost) the use of a string to recognize a function is way worse than using a string to recognize an Event. Just think about it: what happens if someone in the team decides to refactor the code where the calling function is defined and decides to rename the function or delete it?

I tell you what happens: the compiler will not warn the poor programmer of the mistake is doing and even worse, the code will just run as nothing happened. When the error will be found, it could be already too late.

Even worse is the fact that the calling function can be declared as private since the system uses reflection. You know what I usually do when I find a private function that is not used inside the declaring class? I just delete it, because the code is telling me: this function cannot be used outside this class and I am not using it, so why keep useless code?

TL;DR

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:

SendMessage and delegates are used for similar purposes, but they work entirely differently. Delegates/Events behave exactly like they do in any other .Net program. SendMessage() is called much like Unity calls other messages like Update(). Unity reflects over all the code on the game object to see if it can invoke any messages.

As far as performance. Delegates are several times faster than SendMessage. In the order of performance:

(Least) Direct Method Call ==> Delegate/Event ==> Update() ==> SendMessage (Most Expensive)
As far as usage, SendMessage is like a very broad direct method call. The general paradigm is that one script specifically wants to send a message to a specific object. We're just not sure how many times is can respond or if it can respond at all. We know the intended receiver, we just don't know if it can receive the message.

With events, I usually follow the idea that the event owner doesn't know who or how many objects might respond. We are simply telling the world that something happened without too much knowledge of who will use this information.

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:

Now that we’ve established which is faster—interfaces—the question becomes: should you bother? Interfaces are definitely more work to set up, harder to read, more error-prone, and more difficult to integrate into your code. The percentages faster are impressive, but will you save a lot of total CPU time? The above test results are measured in milliseconds per 100 million callback calls. That’s clearly way more than you’ll have in any reasonable game or app.

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).

[jwittner]

I think there are multiple questions here:

1. Should we always use event/delegate going forward? Probably, I certainly prefer it.
2. Should we go back and add event/delegate functionality to SendMessage systems? A nicety, but I would definitely encourage it.
3. Should we drop support for existing SendMessage systems? Breaks old functionality to remove so I'd advise against.

[StephenHodgson]

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.

[izatar]

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.

[StephenHodgson]

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%.

• Donald Knuth

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.

[izatar]

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?

[StephenHodgson]

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.

[stbertou]

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 ?

[StephenHodgson]

@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.

[NeerajW]

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.

[NeerajW]

Path forward for this feature:

1. This change is a breaking change for several apps.
2. We have not collectively agreed on a the right design for the change. Delegates Vs Interface.

Here's what I recommend we do for such features as documented in Contributing.md:

1. We will create a new branch called Feature-SendMessageRemoval.
2. @HodgsonSDAS please feel free to code this change in that branch. Only changes related to this feature will be accepted in that branch. Let's try to lock on the design as well.
3. We will give developers 2 weeks to move to the new feature once the PR's are accepted in that branch.
4. We will announce on forums and other sources where we can.
5. Then we will merge this breaking change after 2 weeks into master.
6. Feature-SendMessageRemoval branch will then be deleted.

Questions/concerns, please let me know.

[NeerajW]

@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!

[StephenHodgson]

@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.

[StephenHodgson]

http://jacksondunstan.com/articles/3605

[keveleigh]

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.

[StephenHodgson]

@keveleigh I think it's more about readability and debug ability more than anything, I just wanted to share that's all.

[keveleigh]

@HodgsonSDAS Ahh yep, definitely agreed with you there! It is interesting to see the data as well. Thanks for the share!

[StephenHodgson]

Closed by #277