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

Fix #321 NRE when Event Reference Listeners event reference is not set #340

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

ThimoDEV
Copy link
Collaborator

As suggested by @soraphis changed that the Event reference listener doesn't just throw a null reference exception everywhere. I added a check for Atoms debug mode. In debug mode the user gets notified of the null reference which he has. But in runtime/production this exception doesn't get thrown.

I chose for a NRE because on clicking this error the connected gameObject gets pinged. This doesn't happen when using the Unity Debug class.

@soraphis
Copy link
Collaborator

soraphis commented Mar 13, 2022

This doesn't happen when using the Unity Debug class.

Debug.Log can take a second arguments. you can pass the object to highlight as second parameter.

I don't think it should throw an exception - even in debug mode - because it can be null ON PURPOSE which would mean that a users script only works if not in debug mode.

with throwing the NRE in our scripts, we take away the oportunity to null check for users of the library. when returning null it might throw a NRE in the users code - but this isn't much of a problem for the user to fix, he can just check if this getter has returned null and act accordingly.

when throwing a NRE he cannot simply check, but instead is forced to use a try-catch block

@ThimoDEV
Copy link
Collaborator Author

with throwing the NRE in our scripts, we take away the oportunity to null check for users of the library. when returning null it might throw a NRE in the users code - but this isn't much of a problem for the user to fix, he can just check if this getter has returned null and act accordingly.

Ah yes this makes a lot of sense to not choose for throwing an exception.

The only problem I have right now, I can't find the gameObject that throws the error/log/warning from within AtomEventReference class. Do you have a suggestion where or how I can find this?

@soraphis
Copy link
Collaborator

soraphis commented Mar 14, 2022

The only problem I have right now, I can't find the gameObject that throws the error/log/warning from within AtomEventReference class. Do you have a suggestion where or how I can find this?

why even log something? ¯\_(ツ)_/¯
the same applies: if the user code want's to check if it is null you're now spamming him with this log message in debug mode.
if you remove the logging altogether, a NRE will be thrown in his own code, providing him all information needed.

if you want to supply the developer with additional information about unassigned references it could be more helpful to create an editor extension, that shows either a) in the inspector that a field must be designed to work properly (e.g. 'required' in https://github.com/dbrizov/NaughtyAttributes#required) or b) show somewhere that (and how many) of those game objects with missing required assignments exist in the scene. 🤔

both would have the additional benefit of showing it earlier (while developing) instead logging in playmode

@ThimoDEV
Copy link
Collaborator Author

The unassigned refererences part seems quite a useful thing to implement for Atoms in my opinion. Or else users might be searching which of their objects has a missing reference. I think its something for V5 as a nice addition. For now I removed the logging.

@soraphis soraphis requested a review from miikalo March 14, 2022 16:10
@miikalo miikalo added this to the v4.4.4 milestone Mar 15, 2022
@miikalo miikalo merged commit 5c094f1 into unity-atoms:canary Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] NullReferenceException when Event Reference Listeners event reference is not set
3 participants