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

[Codechange] Overhauled the network code #991

Merged
merged 6 commits into from Jun 1, 2016

Conversation

Projects
None yet
3 participants
@ulteq
Contributor

ulteq commented May 28, 2016

Fixes the following bugs:

  • Character moves twice as fast in multiplayer
  • Game script messages cause stuttering
  • Network congestion causes severe stuttering
  • Sending outdated stream data when newer data is already available
  • Random crashes / freezes when trucks are removed (Fixes: #962, #963)
  • Texture 'us.png' not found warning in the main menu
  • Player list not updated in the main menu
  • Heavy stuttering of remote trucks

Features:

  • Fully asynchronous network communication

Needs a code review and some in-depth testing.

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 30, 2016

Member

Yess! You did it, man! 💯 👏 😃

I'll need a while to make a thorough review, but just by the looks of the RoR::Network API + the changelog vs. +/- statistics, it looks awesome.
You should add yourself to the Network.{h/cpp} license blocks, you earned it.

Member

only-a-ptr commented May 30, 2016

Yess! You did it, man! 💯 👏 😃

I'll need a while to make a thorough review, but just by the looks of the RoR::Network API + the changelog vs. +/- statistics, it looks awesome.
You should add yourself to the Network.{h/cpp} license blocks, you earned it.

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou May 31, 2016

Contributor

@ulteq Comments are in random order and none of them are particular issues. Just thoughts I had while going through the code.

  • I still can't get to like the implementation of using free functions in a namespace as interface and using static global variables inside the module to represent internal state. First, initialization order of these variables is undefined. It doesn't matter in this case and likely never will. Still, it is a potential gotcha that could be really nasty to track down, if it bites us in the future. The second thing that irks me, is that while intialization order itself is undefined, all global statics are guaranteed to be initialized on startup before entering main. Sure, this won't cause any noticable difference in startup time, however, it still bothers me, that even when not playing over network, memory is being used for network stuff.

  • Use named enum type instead of int for message type in SendMessage. Also return bool instead of int

  • Style suggestion: Replace

    header_t head;
    memset(&head, 0, sizeof(header_t));
    head.command = type;
    head.source = m_uid;
    head.size = len;
    head.streamid = streamid;
    

    with

      header_t head = {
          .command = type,
          .source = m_uid,
          .size = len,
          .streamid = streamid
      };
    
  • https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L171-L172 Redundant memset

  • BTW is SendMessageRaw threadsafe (that is, is SocketW threadsafe)? It dosent't matter here if I see it correctly. Just curious.

  • What is the rational of sleeping here https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L256 ? Other than that threading looks fine to me.

  • https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L282-L299 Are these basically unused? Also NETQUALITY stuff doesn't seem to be really used?

  • Another style suggestion:

    auto user = std::find_if(begin(m_users), end(m_users), [&](auto &u){ return static_cast<int>(u.uniqueid) == header.source; });
    if (user != end(m_users)) {
        Ogre::UTFString msg = RoR::ChatSystem::GetColouredName(user->username, user->colournum) + RoR::Color::CommandColour + _L(" left the game");
        const char *utf8_line = msg.asUTF8_c_str();
        header_t head = {
            .command = MSG2_UTF_CHAT,
            .source  = -1,
            .size    = (int) strlen(utf8_line)
        };
        QueueStreamData(head, (char *)utf8_line);
        LOG(Ogre::UTFString(user->username) + _L(" left the game"));
        m_users.erase(user);
    }
    
Contributor

mikadou commented May 31, 2016

@ulteq Comments are in random order and none of them are particular issues. Just thoughts I had while going through the code.

  • I still can't get to like the implementation of using free functions in a namespace as interface and using static global variables inside the module to represent internal state. First, initialization order of these variables is undefined. It doesn't matter in this case and likely never will. Still, it is a potential gotcha that could be really nasty to track down, if it bites us in the future. The second thing that irks me, is that while intialization order itself is undefined, all global statics are guaranteed to be initialized on startup before entering main. Sure, this won't cause any noticable difference in startup time, however, it still bothers me, that even when not playing over network, memory is being used for network stuff.

  • Use named enum type instead of int for message type in SendMessage. Also return bool instead of int

  • Style suggestion: Replace

    header_t head;
    memset(&head, 0, sizeof(header_t));
    head.command = type;
    head.source = m_uid;
    head.size = len;
    head.streamid = streamid;
    

    with

      header_t head = {
          .command = type,
          .source = m_uid,
          .size = len,
          .streamid = streamid
      };
    
  • https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L171-L172 Redundant memset

  • BTW is SendMessageRaw threadsafe (that is, is SocketW threadsafe)? It dosent't matter here if I see it correctly. Just curious.

  • What is the rational of sleeping here https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L256 ? Other than that threading looks fine to me.

  • https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L282-L299 Are these basically unused? Also NETQUALITY stuff doesn't seem to be really used?

  • Another style suggestion:

    auto user = std::find_if(begin(m_users), end(m_users), [&](auto &u){ return static_cast<int>(u.uniqueid) == header.source; });
    if (user != end(m_users)) {
        Ogre::UTFString msg = RoR::ChatSystem::GetColouredName(user->username, user->colournum) + RoR::Color::CommandColour + _L(" left the game");
        const char *utf8_line = msg.asUTF8_c_str();
        header_t head = {
            .command = MSG2_UTF_CHAT,
            .source  = -1,
            .size    = (int) strlen(utf8_line)
        };
        QueueStreamData(head, (char *)utf8_line);
        LOG(Ogre::UTFString(user->username) + _L(" left the game"));
        m_users.erase(user);
    }
    
@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 31, 2016

Contributor

BTW is SendMessageRaw threadsafe (that is, is SocketW threadsafe)?

Yes it is.

It dosent't matter here if I see it correctly.

Correct.

What is the rational of sleeping here https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L256 ?

To avoid network congestion. It seems to work out very well so far in our multiplayer tests.

https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L282-L299 Are these basically unused?

No, they are now handled elsewhere (mostly in the factories).

Also NETQUALITY stuff doesn't seem to be really used?

Yes, it was also unused in the old code. But it seemed to be useful enough to not just remove it.

header_t head = {
      .command = type,
      .source = m_uid,
      .size = len,
      .streamid = streamid
  };

-> sorry, unimplemented: non-trivial designated initializers not supported

Contributor

ulteq commented May 31, 2016

BTW is SendMessageRaw threadsafe (that is, is SocketW threadsafe)?

Yes it is.

It dosent't matter here if I see it correctly.

Correct.

What is the rational of sleeping here https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L256 ?

To avoid network congestion. It seems to work out very well so far in our multiplayer tests.

https://github.com/ulteq/rigs-of-rods/blob/network-revamp/source/main/network/Network.cpp#L282-L299 Are these basically unused?

No, they are now handled elsewhere (mostly in the factories).

Also NETQUALITY stuff doesn't seem to be really used?

Yes, it was also unused in the old code. But it seemed to be useful enough to not just remove it.

header_t head = {
      .command = type,
      .source = m_uid,
      .size = len,
      .streamid = streamid
  };

-> sorry, unimplemented: non-trivial designated initializers not supported

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr Jun 1, 2016

Member

@mikadou Thanks for the comments

I still can't get to like...

  • I'm not really opposed to transforming this into a class sometime in the future. Back when the code was obfuscated by the Stream* stuff, I was worried the networking is used in too many places (usual case in RoR) and passing one more pointer everywhere would be more trouble than it's worth. Now that I'm looking at the cleaned code, it's not that bad after all.
  • Is it the always-reserved memory that bothers you, or it's potentially useless initialization? In principle, you're right about this, but in this specific case, the amount of inits is little and unlikely to rise much. It's code, we can refactor later.
Member

only-a-ptr commented Jun 1, 2016

@mikadou Thanks for the comments

I still can't get to like...

  • I'm not really opposed to transforming this into a class sometime in the future. Back when the code was obfuscated by the Stream* stuff, I was worried the networking is used in too many places (usual case in RoR) and passing one more pointer everywhere would be more trouble than it's worth. Now that I'm looking at the cleaned code, it's not that bad after all.
  • Is it the always-reserved memory that bothers you, or it's potentially useless initialization? In principle, you're right about this, but in this specific case, the amount of inits is little and unlikely to rise much. It's code, we can refactor later.
@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Jun 1, 2016

Contributor

Is it the always-reserved memory that bothers you, or it's potentially useless initialization?

Both 😄 I agree this is not really an issue here, though.

Can we merge this then? It seems to me that @ulteq has tested this enough to include it.

Contributor

mikadou commented Jun 1, 2016

Is it the always-reserved memory that bothers you, or it's potentially useless initialization?

Both 😄 I agree this is not really an issue here, though.

Can we merge this then? It seems to me that @ulteq has tested this enough to include it.

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr Jun 1, 2016

Member

Can we merge this then? It seems to me that @ulteq has tested this enough...

👍

Member

only-a-ptr commented Jun 1, 2016

Can we merge this then? It seems to me that @ulteq has tested this enough...

👍

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Jun 1, 2016

Contributor

All right.

Contributor

ulteq commented Jun 1, 2016

All right.

@ulteq ulteq merged commit c22fd60 into RigsOfRods:master Jun 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ulteq ulteq deleted the ulteq:network-revamp branch Jun 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment