test: Companion Tests for MTT-640 and PR-749 #754
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the first pass changes required to implement MTT-640. The default player prefab is now a directly assigned value as opposed to a checkbox in the network prefab list. The default player prefab has to also be included in the NetworkPrefab list (this is so the default player prefab can be overridden too) NetworkPrefab entries can be overridden and directly assigned alternate prefabs for clients to spawn. The source prefab (what the server uses) can be a prefab reference or a prefab hash.
This update is the first pass of making this update as painless as possible to the user by "auto-handling" the v0.1.0 NetworkPrefab pattern as well as there is additional "user-bumper" code that assures the assigned default player prefab is always in the NetworkPrefab list.
Removing the auto insert within the editor and doing this at runtime. Added additional check to handle assigning the OverridingSourcePrefab if it is null and the Prefab is not.
This will prevent the scenario where a user might want to delete the default player prefab but also has it assigned within the NetworkPrefab list. This assures that a newly created network prefab list entry is blank and that there are not two entries within the list that have the same global object id hash.
just adding a few comments
Adjusted names of the new NetworkPrefab properties and added additional comments to better clarify some of the "trickery" happening that will help make this a "non-breaking" feature when it comes to existing v0.1.0 NetworkConfigs and the already selected default player prefab.
This will prevent any future client side soft synchronization issues or related issues where only one (or a few) NetworkObjects are misconfigured and the client fails to synchronize (find or instantiate). As opposed to completely breaking the rest of the pending NetworkObjects to be synchronized/instantiated, it will log an error and continue loading. This also has some additional comments and updates to comments for better clarity.
minor change in comments
This removes the CreatePlayerPrefab property from the NetworkConfig and from the NetworkManagerEditor. It is no longer needed as the NetworkConfig.PlayerPrefab property being something other than null tells us that the user wants to have MLAPI automatically spawn the players for them.
removed unused namespace references.
Added a tooltip for the new NetworkConfig.PlayerPrefab property
Added some spaces between the forward slashes and comment's text body
Migrating some useful elements from theScaleDemo repo into the test project for manual testing purposes. Added a samples folder with the first sample: Enalbe or Disable in-scene placed NetworkObject. Did some general clean up of the folders.
This fixes an issue that can occur with serialization and NetworkPrefabOverrideLinks. The policy is to ALWAYS clear this dictionary before building the links dictionary.
Adding check for NetworkManager before trying to access it.
NoelStephensUnity
changed the title
This removes the NetworkConfig compatibility checks that auto-upgraded a previous NetworkConfig setting to the new pattern. Updated NetworkPrefab according to this change, isPlayer is no longer needed. Added additional code to NetworkPrefab that will return a hash reflective of the override state. Adjusted some comments.
This removes the NetworkConfig compatibility checks that auto-upgraded a previous NetworkConfig setting to the new pattern. Updated NetworkPrefab according to this change, isPlayer is no longer needed. Added additional code to NetworkPrefab that will return a hash reflective of the override state. Adjusted some comments.
Did some renaming and clean up for the newly added assets. Verified working.
Some minor adjustments and asset additions that will help guide manual testing.
Minor adjustment for the right GlobalObjectIdHash check.
Moving manual scripts into the default ManualTests/Scripts folder.
Made the NetworkPrefab internal as well as NetworkPrefabOverride. Removed GetNetworkPrefabIndexOfHash and GetPrefabHashFromIndex as they were used in the old hash generation pattern. Updated comments a bit.
A bunch of adjustments in the menu system. Added command line handling. This includes a fix for the "odd behavior" with GlobalObjectIdHash and the HandleApproval method in NetworkManager. Unity Chan animator only requires one to press "c" once and it will animate until the end of the animation. New SceneMenuReference for scenes that are sub-menus to the main menu. Some bug fixes in various places (typically on exiting a scene).
This just fixes the fix for the weird GlobalObjectIdHash issue that only occurs under a very specific scenario.
Still needs review, but had a few "seemingly new" edge case scenarios pop up where editor to stand alone connections could be problematic under various scenarios where the OnValidate method could be invoked at the wrong time. Added some debug information to help narrow down when OnValidate was being invoked relative to when the GlobalObjectIdHash was being assigned. Added command line handler functionality similar to what we used in scaledemo. To test these fixes just build and run through all of the different available scenes under the following scenarios: Editor Host to stand alone build based Client Editor Client to stand alone build based Host Stand alone build based Client/Host to stand alone build based client/host. Editor Host/Client to Editor Host/Client All cases should yield no errors or invalid GlobalObjectIdHash values (especially editor to stand alone)
Finished the scene and menu referencing pattern. Now both menu systems use the same set of code and now there is a MenuReference and SceneReference that can be used in the MainMenuManager and SceneMenuManager (which really only define the reference type to use) Minor tweaks in materials. Remove some NetworkObject related debug logging.
This includes some additional checks for prefabs as well as added define for testing this specific fix regarding in-scene placed network prefabs. This includes prefab creation and unique GlobalObjectIdHash assignment checks as well.
0xFA11
reviewed
Apr 26, 2021
| internal uint GlobalObjectIdHash; | ||
|
|
||
| #if UNITY_EDITOR | ||
| private void OnValidate() |
There was a problem hiding this comment.
fixes (we figured today together) for NetworkObject.GlobalObjectIdHash should go (and will go) under a separate PR on its own :)
There was a problem hiding this comment.
Leaving the core fixes in until the other PR is merged, then we can merge that back into this PR and this PR should be good to go.
0xFA11
reviewed
Apr 26, 2021
0xFA11
reviewed
Apr 26, 2021
| /// <summary> | ||
| /// A general object that can be used for testing purposes | ||
| /// </summary> | ||
| public class GenericObject : NetworkBehaviour |
There was a problem hiding this comment.
GenericNetworkObject perhaps?
0xFA11
reviewed
Apr 26, 2021
Comment on lines
+1
to
+4
| public interface IPlayerMovement | ||
| { | ||
| void Move(int speed); | ||
| } |
There was a problem hiding this comment.
I see where this is going and I like it :)
0xFA11
reviewed
Apr 26, 2021
| StartSpawningBoxes(); | ||
| } | ||
| } | ||
| base.NetworkStart(); |
There was a problem hiding this comment.
you don't actually need to call the base method here at all (there's nothing in the base, there never will be).
There was a problem hiding this comment.
I tend to make that a habit in the event we want some "default thing" to happen... but yeah generally speaking as it is today.. it isn't needed.
0xFA11
reviewed
Apr 26, 2021
| m_PrefabPool = objectPool; | ||
| } | ||
|
|
||
| public void Dispose() |
There was a problem hiding this comment.
should we kill this method entirely?
0xFA11
reviewed
Apr 26, 2021
0xFA11
suggested changes
Apr 26, 2021
There was a problem hiding this comment.
apart from leaving GlobalObjectIdHash fixes for a separate PR, it looks very good to me!
I left some minor suggestions, you may or may not address them, they're alright.
also, this branch already proved to be super useful today when we tried to debug/repro issues around GlobalObjectIdHash and I already see these tests growing.
(also, we might finally convince @jeffreyrainy to play with testproject instead of netcode-scaledemo since it now has more or less same functionalities 😛 )
Renamed GenericObject script to GenericNetworkObjectBehaviour
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes additional acceptance tests for MTT-640 and PR-749.
This also includes: