Skip to content

chore!: per-asmdef namespaces instead of per-folder #1009

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 3, 2021
Merged

chore!: per-asmdef namespaces instead of per-folder #1009

merged 6 commits into from
Aug 3, 2021

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Aug 3, 2021

it's a common practice in Unity packages to use the same top-level namespace for types including sub-folders (e.g. Unity Transport, DOTS Netcode etc.).

sticking with the same namespace would make discovery much better.
for instance, you don't have to remember using Unity.Multiplayer.Netcode.NetworkVariable directive to define your NetworkVariable<int> m_Health; anymore, yay!

sticking with top-level namespace would also get rid of long list of using statements subsetting folders.
(take a look at PR diffs for examples)

this PR proposes following top-level namespaces (hence rootNamespace fields in asmdefs):

  • nspace Unity.Multiplayer.Netcode
    asmdef Unity.Multiplayer.MLAPI.Runtime
  • nspace Unity.Multiplayer.Netcode.Prototyping
    asmdef Unity.Multiplayer.MLAPI.Prototyping
  • nspace Unity.Multiplayer.Netcode.Editor
    asmdef Unity.Multiplayer.MLAPI.Editor
  • nspace Unity.Multiplayer.Netcode.RuntimeTests
    asmdef Unity.Multiplayer.MLAPI.RuntimeTests
  • nspace Unity.Multiplayer.Netcode.EditorTests
    asmdef Unity.Multiplayer.MLAPI.EditorTests
  • nspace TestProject.RuntimeTests
    asmdef TestProject.RuntimeTests
  • nspace TestProject.ManualTests
    asmdef TestProject.ManualTests
  • etc.

I think we should be deliberate when hiding types behind namespaces, such as Unity.Multiplayer.Netcode.Transports.UNET (which is the only subset namespace under Runtime asmdef atm).

Hopefully, this'd make UX & optics much better and frictionless.

(note: you could also expect asmdef name changes coming soon!)

0xFA11
0xFA11 commented Aug 3, 2021
View reviewed changes
com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs
Comment on lines -6 to -18
using Unity.Multiplayer.Netcode.Logging;
using Unity.Multiplayer.Netcode.Configuration;
using Unity.Multiplayer.Netcode.Profiling;
using Unity.Multiplayer.Netcode.Serialization;
using Unity.Multiplayer.Netcode.Transports;
using Unity.Multiplayer.Netcode.Connection;
using Unity.Multiplayer.Netcode.Messaging;
using Unity.Multiplayer.Netcode.SceneManagement;
using Unity.Multiplayer.Netcode.Spawning;
using Unity.Multiplayer.Netcode.Exceptions;
using Unity.Multiplayer.Netcode.Serialization.Pooled;
using Unity.Multiplayer.Netcode.Transports.Tasks;
using Unity.Multiplayer.Netcode.Timing;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

0xFA11
0xFA11 commented Aug 3, 2021
View reviewed changes
com.unity.multiplayer.mlapi/Runtime/Profiling/ITransportProfilerData.cs
namespace Unity.Multiplayer.Netcode.Profiling
namespace Unity.Multiplayer.Netcode
Copy link
Contributor Author

@0xFA11 0xFA11 Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might still want to keep profiling-related types behind a namespace like Unity.Multiplayer.Netcode.Profiling, what would you guys think? @becksebenius-unity @Rosme

also, should any of these types be public at all? I though they all could be internal types, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I see that NetworkManager implements IProfilableTransportProvider interface which has a property returning ITransportProfilerData type. I guess, if we were to keep these two public and make everything internal, that'd work fine and we'd still keep them under Unity.Multiplayer.Netcode namespace — how that sounds to you guys? 👀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITransportProfilerData will ultimately become deprecated, it's not something that we're using for Profiler V2. In general I'd say we should make as much of this internal as possible, although this was public because it needs to be used in other assemblies

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aligned with @becksebenius-unity

ShadauxCat
ShadauxCat approved these changes Aug 3, 2021
View reviewed changes
Copy link
Collaborator

@ShadauxCat ShadauxCat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1,000% on board with this.

No... 10,000%.

@0xFA11 0xFA11 enabled auto-merge (squash) August 3, 2021 17:52
jeffreyrainy
jeffreyrainy reviewed Aug 3, 2021
View reviewed changes
com.unity.multiplayer.mlapi/Runtime/Core/SnapshotSystem.cs
@@ -1,12 +1,6 @@
using System;
using System.Collections.Generic;
using System.IO;
using Unity.Multiplayer.Netcode.Messaging;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad to see there's nothing further than that in this file! :-)

@0xFA11 0xFA11 merged commit 9c73270 into develop Aug 3, 2021
@0xFA11 0xFA11 deleted the chore/less-namespaces branch August 3, 2021 18:20
SamuelBellomo added a commit that referenced this pull request Aug 11, 2021
@SamuelBellomo
Merge branch 'develop' into sam/feature/interpolation-for-network-tra…
b776957 
…nsform

* develop: (32 commits)
  refactor: calling networkShow(NetworkObject) code in networkshow(List<NetworkObject>) (#1028)
  feat: snapshot. MTT-685 MTT-822 (#1021)
  test: adding a multi-instance test checking NetworkShow and NetworkHide on lists of objects (#1036)
  fix: corrected NetworkVariable WriteField/WriteDelta/ReadField/ReadDelta dropping the last byte if unaligned. (#1008)
  chore: run standards check over solution files (#1027)
  chore: replace MLAPI with Netcode in Markdown files (#1025)
  fix!: added plainly-callable Add() method to NetworkSet [MTT-1005] (#1022)
  fix: fixing incorrect merge done as part of commit 85842ee (#1023)
  chore: cleanup/upgrade serialized scenes (#1020)
  chore: replace MLAPI with Netcode in C# source files (#1019)
  test: add network collections, struct and class tests MTT-936 (#1000)
  test: add buildtests to test build pipeline on target platforms (#1018)
  chore: rename MLAPI types to Netcode (#1017)
  chore!: rename asmdefs, change top-level namespaces (#1015)
  Replacing community NetworkManagerHUD with a simpler implementation (#993)
  test: network prefab pools and INetworkPrefabInstanceHandler (#1004)
  fix: do not expose Runtime internals to TestProject.ManualTests asmdef (#1014)
  refactor: snapshot. merge preparation. Removing old acks, removing unused varia… (#1013)
  chore!: per-asmdef namespaces instead of per-folder (#1009)
  feat: snapshot. ground work, preparing depedencies. No impact on code behaviour (#1012)
  ...

# Conflicts:
#	com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs
#	com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs
@0xFA11 0xFA11 mentioned this pull request Sep 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShadauxCat ShadauxCat ShadauxCat approved these changes

@Rosme Rosme Rosme approved these changes

@jeffreyrainy jeffreyrainy jeffreyrainy approved these changes

@mattwalsh-unity mattwalsh-unity mattwalsh-unity approved these changes

@becksebenius-unity becksebenius-unity becksebenius-unity approved these changes

@NoelStephensUnity NoelStephensUnity Awaiting requested review from NoelStephensUnity NoelStephensUnity is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@0xFA11 @ShadauxCat @Rosme @jeffreyrainy @mattwalsh-unity @becksebenius-unity