-
Notifications
You must be signed in to change notification settings - Fork 450
fix: applying CheckObjectVisibility upon NetworkShow #2463
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
Conversation
I'm not 100% we want this as it modifies the API slightly. One could argue either for or against this. Various arguments that could be made:
What are the thoughts of other members of the team? |
I think this PR helps remove user confusion with the two potentially conflicting visibility directives. By providing some form of feedback (i.e. warning) users can more easily come to the conclusion that their object's visibility delegate is still returning false and make the appropriate adjustments in their implementation. 👍 |
@@ -331,6 +331,15 @@ public void NetworkShow(ulong clientId) | |||
throw new VisibilityChangeException("The object is already visible"); | |||
} | |||
|
|||
if (CheckObjectVisibility != null && !CheckObjectVisibility(clientId)) | |||
{ | |||
if (NetworkManager.LogLevel == LogLevel.Developer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this message log if NetworkManager.LogLevel <= LogLevel.Normal
?
A user would be more likely to see this message and it wouldn't be something that would get spammed all of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the optional suggested modification to the loglevel.
The rest looks good.
Co-authored-by: Fatih Mar <mfatihmar@gmail.com>
With this change, what's the intended way of creating a NetworkObject that's hidden from all clients by default (even those who connect after the object has been spawned) but has the ability to be shown to a specific client at a later time? The simplest way I can think to do it is as follows, which does seem to work but is a little strange: Spawning the NetworkObject, hidden from all clients by default:
Showing the NetworkObject to a specific
Hiding the NetworkObject from
Perhaps it would make sense to have some kind of Spawning the NetworkObject, hidden from all clients by default:
Showing the NetworkObject to a specific
Hiding the NetworkObject from
|
@A1win
It isn't the "simplest" in regards to lines of code, but it shows the general flow of how you can start out with a specific visibility state and make client specific visibility changes. You can also just use If you are wanting something to simply be "not visually visible" but still maintain an instance on the client side, then using _In order to do that, I would just disable the visual elements but not disable any GameObject with a NetworkObject or any NetworkBehaviours. Typically under this scenario I recommend placing all visual elements on a child under the root GameObject (not parented netcode relative, but when creating the prefab) while keeping all netcode related components on the root or other children...so all you need to do there is just disable the "child visual components GameObject" and then re-enable it when you are ready for it to be visible. Of course, you would need to track the "visible" state in any NetworkBehaviour updates and exit early if it was "hidden". _
|
I did mean network visibility, and as long as calling NetworkHide on an object immediately after spawning it won't even briefly show it to the wrong clients, something along the lines of the code you posted is probably good enough. 👍 |
The name of the class is fubar (just re-read it), but yeah that class should do the trick (might rename it to whatever makes sense to you). |
Dang, I'm late to the debate lol... But why not just a simple default boolean in the NetworkShow method... |
Not on the team, but I concur, on the second point. I am manually calling the visibility due to a change in the application/gameplay. Right now since the NGO API modification, it kinda blocks my choice. Since all objects are visible by default, its just a lot of bandwidth for the spawns on the client side for objects I do not want to be visible. So I use the check object delegate to return false on all objects. Then use the NetworkShow to manually adjust by my needs. I do have several objects that use different implementations for the delegate besides returning false. So I do think they are the same feature but they have different uses per developer and use case. Also I have a lot of dynamic objects spawning and despawning from the client so, its just full control for me to decide how to design. I propose a simple default boolean so the developer can choose to skip the "CheckObjectVisibility" delegate when manually calling "NetworkShow" from the network object. |
Prevents
NetworkShow
from overridingCheckObjectVisibility
.Found while investigating #2454 which is already fixed.