Announcement

Collapse
No announcement yet.

[Fix Incoming] Seriously damaging change to Online Subsystems in 4.20

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    #16
    Hey all,

    We're looking at getting a fix in for this. I don't have an ETA for you at this time, but stay tuned.

    Thanks for bringing this to our attention.

    ~Tim

    Comment


      #17
      Apologies for breaking your custom online subsystem implementations. Two features that improved UniqueId overall came together to break the code in question.

      1) UniqueId compression - up until now, in the interest of simplicity, we were simplify passing a string across the network. This was grossly inefficient, but for small player counts it wasn't on our radar. For Epic's unique id scheme, a guid, it was 43 bytes to replicate. We found that we could pack this into nibbles and save a ton of space. The code now serializes at 18 bytes, which is significant savings in FNBR where we replicate 100 user ids to everyone (10k replications * 43 bytes). Additionally for cross play we replicate other platform unique ids (10k * 64bit number in string format). These weren't as big a savings, but since they were straight up integers, we could pack them the same way.

      2) UniqueId type - again for crossplay, we needed to better understand what type of UniqueId we were dealing with without having to cross reference other systems. Replicating the type without losing savings from 1 is the reason for the "hash/index" value that is causing you trouble. 5 bits of the packet were used to have room for 32 online subsystems. Of course this means we need to know ahead of time all the values. This is clearly not conducive to custom implementations.

      We are going to fix this. We will use one of the bits to signify a custom Online Subsystem, and pack the name of your system into the packet. We thought about some way to deterministically register all known online subsystems, but this won't work given that not all will register in all scenarios (not all platforms register all subsystems). The simplest thing to do is sacrifice a little bit of the compression savings for simplicity and convenience.

      We are looking to get an implementation in ASAP and have it available with the one of the next 4.20 hotfix releases.

      Apologies for the delay in a response, we do not often get a lot of visibility into specific forum issues. I hope this addresses the concerns, the online team does care about the pains of other developers.

      Comment


        #18
        Thank you Crzyhomer for (what is for this forum's standard) a very swift and very detailed response. Looking forward to the fix!
        [Submitted] Advanced Data Validation

        Comment


          #19
          Originally posted by Crzyhomer View Post
          We are looking to get an implementation in ASAP and have it available with the one of the next 4.20 hotfix releases.

          Apologies for the delay in a response, we do not often get a lot of visibility into specific forum issues. I hope this addresses the concerns, the online team does care about the pains of other developers.
          Thanks for a detailed response, this certainly explains a lot! I'm glad that ultimately the new code will result in some net optimizations. Also, big thanks for (hopefully) squeezing it into a Hotfix!

          Editing the title to reduce pitchfork-ness.
          Last edited by TheJamsh; 08-25-2018, 07:12 AM.

          Comment


            #20
            Any updates on this? I believe we are running into this and its critically holding up development.

            Cheers!

            Comment


              #21
              This was logged as https://issues.unrealengine.com/issue/UE-63326 and believed to be fixed with the 4.20.3 hotfix, however new information is showing that may not be the case.

              I've pulled some conversation from private channels that may help.

              For those using source code via GitHub:

              If you change the serialization of the type in both places to serialize it as a string it should work.

              In FUniqueNetIdRepl::MakeReplicationData, change the two places Type is serialized from
              1. if (TypeHash == TypeHash_Other)
              2. {
              3. Writer << Type;
              4. }

              to:
              1. if (TypeHash == TypeHash_Other)
              2. {
              3. FString TypeString = Type.ToString();
              4. Writer << TypeString;
              5. }

              In FUniqueNetIdRepl::NetSerialize, change the two places Type is serialized from
              1. if (TypeHash == TypeHash_Other)
              2. {
              3. Ar << Type;
              4. if (Ar.IsError() || Type == NAME_None)
              5. {
              6. bValidTypeHash = false;
              7. }
              8. }

              to:
              1. if (TypeHash == TypeHash_Other)
              2. {
              3. FString TypeString;
              4. Ar << TypeString;
              5. Type = *TypeString;
              6. if (Ar.IsError() || Type == NAME_None)
              7. {
              8. bValidTypeHash = false;
              9. }
              10. }
              For binary users, this might be a viable workaround
              If you happen to use FUniqueNetIdString in your custom online subsystem then it might be possible. You could provide one of the compiled in FNames of an online subsystem to the Type paramter of the FUniqueNetIdString ctor in your online subsystem. I tested this with "LIVE" in our case. If this built in subsystem is not loaded the serialization code will fall back to FUniqueNetIdString. ("LIVE" is a good match in this case as it also uses FUniqueNetIdString so it doesn't matter if it's loaded or not.)
              Stephen Ellis | Enterprise Program Coordinator

              Comment


                #22
                Does anyone know if the solution above for binary users works (particularly with the UWorks plugin)?

                Comment


                  #23
                  Originally posted by Vormulac View Post
                  Does anyone know if the solution above for binary users works (particularly with the UWorks plugin)?
                  I've been following this thread.

                  The binary "workaround" is not a solution for V13 (and below) of UWorks. I didn't know we would reach this point so I didn't use FUniqueNetIdString. I'm looking into modifying this part for the next version.

                  That being said, I find both of these solution ugly. It's already a pain to even create a new, blank subsystem (the module loads subsystems by creating factories which create subsystems which create interfaces... Now we have to take care of hashes and so on... What? And that's just the tip).

                  Comment


                    #24
                    I don't see this being fixed at all, despite the issue tracking having it marked as "FIXED": https://issues.unrealengine.com/issue/UE-63326

                    Here's the relevant code from "OnlineSubsystemUtilsModule.cpp":

                    Code:
                    private:
                    
                        void Init()
                        {
                            FOnlineSubsystemDelegates::OnOnlineSubsystemCreated.AddRaw(this, &FOnlineSubsystemUtils::OnOnlineSubsystemCreated);
                            CreateNameHashes();
                        }
                    
                        void CreateNameHashes()
                        {
                            int32 HashId = 1;
                    #define CREATE_HASH(MacroName) \
                    { \
                     uint8 Hash = static_cast<uint8>(HashId); \
                     SubsystemNameToHash.Add(MacroName, Hash); \
                     HashToSubsystemName.Add(Hash, MacroName); \
                     HashId++; \
                    }
                            CREATE_HASH(NULL_SUBSYSTEM); 
                            CREATE_HASH(MCP_SUBSYSTEM);
                            CREATE_HASH(STEAM_SUBSYSTEM);
                            CREATE_HASH(PS4_SUBSYSTEM);
                            CREATE_HASH(LIVE_SUBSYSTEM);
                            CREATE_HASH(GOOGLE_SUBSYSTEM);
                            CREATE_HASH(GOOGLEPLAY_SUBSYSTEM);
                            CREATE_HASH(FACEBOOK_SUBSYSTEM);
                            CREATE_HASH(IOS_SUBSYSTEM);
                            CREATE_HASH(TENCENT_SUBSYSTEM);
                            CREATE_HASH(SWITCH_SUBSYSTEM);
                            CREATE_HASH(AMAZON_SUBSYSTEM);
                            CREATE_HASH(GAMECIRCLE_SUBSYSTEM);
                            CREATE_HASH(THUNDERHEAD_SUBSYSTEM); 
                            CREATE_HASH(WECHAT_SUBSYSTEM); 
                            CREATE_HASH(TWITCH_SUBSYSTEM); 
                            CREATE_HASH(OCULUS_SUBSYSTEM); 
                            CREATE_HASH(QUAIL_SUBSYSTEM);
                            // Shouldn't need these as they are mocking interfaces for existing platforms
                            CREATE_HASH(PS4SERVER_SUBSYSTEM);
                            CREATE_HASH(LIVESERVER_SUBSYSTEM);
                    
                    #undef CREATE_HASH
                    
                            ensure(SubsystemNameToHash.Num() == (HashId - 1));
                            ensure(HashToSubsystemName.Num() == (HashId - 1));
                    
                            // FUniqueNetIdRepl uses 5 bits to transmit the HashId and 31 is used for OnlineSubsystems not included in this list
                            check(HashId < 31);
                        }
                    
                        /** If false it will not try to do online PIE at all */
                        bool bShouldTryOnlinePIE;
                    
                        /** Delegate set by the engine for notification of external UI operations */
                        FOnExternalUIChangeDelegate OnExternalUIChangeDelegate;
                        TMap<FName, FDelegateHandle> ExternalUIDelegateHandles;
                    
                        /** Delegate binding when new online subsystems are created */
                        FDelegateHandle OnOnlineSubsystemCreatedDelegateHandle;
                        /** Mapping of OSS Names to uint8 hashes */
                        TMap<FName, uint8> SubsystemNameToHash;
                        TMap<uint8, FName> HashToSubsystemName;
                    
                        friend FOnlineSubsystemUtilsModule;
                    };
                    As you can notice, the function "CreateNameHashes()" and the variables "SubsystemNameToHash" and "HashToSubsystemName" are still private. If you want to allow plugins to host custom subsystems, as you should, this is where the change needs to happen.

                    Just add a public function "AddNameHash" instead of that hideous macro.

                    Comment


                      #25
                      Has this been fixed with the release of 4.21?
                      Waiting for clean fix unitil I purchase, so can't check myself

                      Comment


                        #26
                        It appears to be fixed in 4.21. The changes described in post #21 above have been applied in 4.21, in the fix changelist linked from https://issues.unrealengine.com/issue/UE-63326.

                        I haven't tested it myself yet though.

                        Comment


                          #27
                          Originally posted by Paul &quot;TBBle&quot; Hampson View Post
                          It appears to be fixed in 4.21. The changes described in post #21 above have been applied in 4.21, in the fix changelist linked from https://issues.unrealengine.com/issue/UE-63326.

                          I haven't tested it myself yet though.
                          I have. Couldn't get it work.

                          Stephen Ellis Would you please take a look at post #24? I have tested this with a custom OnlineSubsystem hosted in a plugin, several times. You can take a look at my product from my subscription and use the exact files from the Marketplace to reproduce. Thanks in advance!

                          Comment


                            #28
                            Originally posted by vlad.serbanescu11 View Post

                            I have. Couldn't get it work.

                            Stephen Ellis Would you please take a look at post #24? I have tested this with a custom OnlineSubsystem hosted in a plugin, several times. You can take a look at my product from my subscription and use the exact files from the Marketplace to reproduce. Thanks in advance!
                            Hi Vlad.

                            Our engineers have reviewed the issue, and in our tests the functionality is working as expected, so you may have something unique to your case that isn't covered by our tests. In order for us to investigate this fully, please submit a bug report via https://epicsupport.force.com/unrealengine/s/. Please be sure to include as much information as possible, including the exact failure, and ideally a test project that reproduces the issue.

                            Thank you.
                            Stephen Ellis | Enterprise Program Coordinator

                            Comment


                              #29
                              Originally posted by Stephen Ellis View Post

                              Hi Vlad.

                              Our engineers have reviewed the issue, and in our tests the functionality is working as expected, so you may have something unique to your case that isn't covered by our tests. In order for us to investigate this fully, please submit a bug report via https://epicsupport.force.com/unrealengine/s/. Please be sure to include as much information as possible, including the exact failure, and ideally a test project that reproduces the issue.

                              Thank you.
                              In 4.21, even with the changes, when we try to call UniqueID.IsValid() inside of AGameModeBase::PreLogin(), when adding a breakpoint, the uniqueID is always nil. It's just empty. It then crashes because it says "incompatible_net_id" or something.

                              Comment


                                #30
                                Stephen Ellis OnlineReplStructs.cpp, #237

                                Code:
                                // Non empty and hex encoded
                                uint8 TypeHash = GetTypeHashFromEncoding(EncodingFlags);
                                if (TypeHash == 0)
                                {
                                    // If no type was encoded, assume default
                                    TypeHash = UOnlineEngineInterface::Get()->GetReplicationHashForSubsystem(UOnlineEngineInterface::Get()->GetDefaultOnlineSubsystemName());
                                }
                                FName Type;
                                bool bValidTypeHash = TypeHash != 0;
                                Why? My one single question? Just why do you "assume default"? If I have an "OnlineSubsystemStephen" and you can't find the hash for "Stephen", why do you just assume "Null" instead? By what logic is that supposed to work for custom subsystems?

                                Here's how the "UOnlineEngineInterface::Get()" part works. I don't see any trace of loading my config's subsystem anywhere.

                                Code:
                                UOnlineEngineInterface* UOnlineEngineInterface::Get()
                                {
                                    if (!Singleton)
                                    {
                                        // Proper interface class hard coded here to emphasize the fact that this is not expected to change much, any need to do so should go through the OGS team first
                                        UClass* OnlineEngineInterfaceClass = StaticLoadClass(UOnlineEngineInterface::StaticClass(), NULL, TEXT("/Script/OnlineSubsystemUtils.OnlineEngineInterfaceImpl"), NULL, LOAD_Quiet, NULL);
                                        if (!OnlineEngineInterfaceClass)
                                        {
                                            // Default to the no op class if necessary
                                            OnlineEngineInterfaceClass = UOnlineEngineInterface::StaticClass();
                                        }
                                
                                        Singleton = NewObject<UOnlineEngineInterface>(GetTransientPackage(), OnlineEngineInterfaceClass);
                                        Singleton->AddToRoot();
                                    }
                                
                                    return Singleton;
                                }
                                I'm done with investigating this myself. Everything I find points to the fact that the engine can't read a simple config variable and create its own hash based on that. Hard stuff. You'll have a new bug report with an empty subsystem in an empty plugin in an empty project with 100% repro success and another mention here.

                                Comment

                                Working...
                                X