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

[Cleanup] Minor network cleanup #945

Merged
merged 4 commits into from May 19, 2016

Conversation

Projects
None yet
2 participants
@ulteq
Contributor

ulteq commented May 19, 2016

  • Simply run GUI_Multiplayer::update every two seconds
  • Replaced mutex construct with std::atomic
  • Removed duplicate getUID() method
  • Only hold m_send_work_mutex when needed
@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 19, 2016

Member

👎 All the individual items in the commit's description should have been separate commits. For example Net quality: replaced explicit mutex with std::atomic. Remember, I'm reviewing this in my working hours. My brain is under external load 😄

Further, as I already said - right now we don't need minor cleanups, but a plan. We have gEnv->network which mostly serves as "are we in multiplayer?" indicator and Stream* beast to do the actual work. As I outlined here, I have no problem with global state, but global variables are icky - they indicate devs aren't sure what they want to do with the globals and so they can't hide them behind an interface.

Member

only-a-ptr commented May 19, 2016

👎 All the individual items in the commit's description should have been separate commits. For example Net quality: replaced explicit mutex with std::atomic. Remember, I'm reviewing this in my working hours. My brain is under external load 😄

Further, as I already said - right now we don't need minor cleanups, but a plan. We have gEnv->network which mostly serves as "are we in multiplayer?" indicator and Stream* beast to do the actual work. As I outlined here, I have no problem with global state, but global variables are icky - they indicate devs aren't sure what they want to do with the globals and so they can't hide them behind an interface.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 19, 2016

Contributor

All the individual items in the commit's description should have been separate commits.

Agreed.

Further, as I already said - right now we don't need minor cleanups, but a plan.

First throwing away as much junk as possible feel right for me.

Contributor

ulteq commented May 19, 2016

All the individual items in the commit's description should have been separate commits.

Agreed.

Further, as I already said - right now we don't need minor cleanups, but a plan.

First throwing away as much junk as possible feel right for me.

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 19, 2016

Member

First throwing away as much junk as possible feel right for me.

I see, but there's a risk of wasting time throwing junk around instead of out.

Member

only-a-ptr commented May 19, 2016

First throwing away as much junk as possible feel right for me.

I see, but there's a risk of wasting time throwing junk around instead of out.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 19, 2016

Contributor

but there's a risk of wasting time

Yes, but I can at least control the amount of time I spend.

Contributor

ulteq commented May 19, 2016

but there's a risk of wasting time

Yes, but I can at least control the amount of time I spend.

[Cleanup] Removed duplicate 'Network' method
Removed static 'Network::myuid'
@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 19, 2016

Member

Yes, but I can at least control the amount of time I spend.

True, but the measure of time spent shouldn't be 'amount', but 'quality'. If you don't have a plan, then you can't know whether your time spent has a purpose. Specifically with the networking, you can try cleaning it up the way it is, but since the design itself is a mess, you're bound to just keep refactoring back and forth until it settles, somehow. That's a plausible strategy, sure, but we can do better.

Member

only-a-ptr commented May 19, 2016

Yes, but I can at least control the amount of time I spend.

True, but the measure of time spent shouldn't be 'amount', but 'quality'. If you don't have a plan, then you can't know whether your time spent has a purpose. Specifically with the networking, you can try cleaning it up the way it is, but since the design itself is a mess, you're bound to just keep refactoring back and forth until it settles, somehow. That's a plausible strategy, sure, but we can do better.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 19, 2016

Contributor

My plan is to finish this: #936, which means adding a MSG2_STREAM_UNREGISTER message. That's the only reason I'm messing with the current code.

Contributor

ulteq commented May 19, 2016

My plan is to finish this: #936, which means adding a MSG2_STREAM_UNREGISTER message. That's the only reason I'm messing with the current code.

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 19, 2016

Member

My plan is...

In that case you're fragmenting your effort into multiple branches for no good reason. As I said before, I don't see much sense in commiting "test code with 5sec placeholder" first rather than experimenting with the message directly.

Member

only-a-ptr commented May 19, 2016

My plan is...

In that case you're fragmenting your effort into multiple branches for no good reason. As I said before, I don't see much sense in commiting "test code with 5sec placeholder" first rather than experimenting with the message directly.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 19, 2016

Contributor

In that case you're fragmenting your effort into 2-3 PR

Yes, because I don't want to have both cleanup and feature commits in a single PR.

I don't see much sense in commiting "test code with 5sec placeholder" first rather than experimenting with the message directly.

I won't merge the 5 second placeholder.

It's only there to test if NetworkStreamManager::getSingleton().removeStream() works as intended.

Contributor

ulteq commented May 19, 2016

In that case you're fragmenting your effort into 2-3 PR

Yes, because I don't want to have both cleanup and feature commits in a single PR.

I don't see much sense in commiting "test code with 5sec placeholder" first rather than experimenting with the message directly.

I won't merge the 5 second placeholder.

It's only there to test if NetworkStreamManager::getSingleton().removeStream() works as intended.

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 19, 2016

Member

I don't want to have both cleanup and feature commits in a single PR

That's good, but either way, bugfixing and refactoring at the same time doesn't really work. Not because of code organization, but because of mind organization. I'd say you should focus on adding the MSG2_STREAM_UNREGISTER now and then do a full-scale overhaul of the networking. You can do it.

I won't merge the... It's only there to test...

OK, I see your work pattern now. I'll just let you code and stop drawing premature conclusions. However, I suggest you mention "this is WIP" in the PRs so this is clear.

Member

only-a-ptr commented May 19, 2016

I don't want to have both cleanup and feature commits in a single PR

That's good, but either way, bugfixing and refactoring at the same time doesn't really work. Not because of code organization, but because of mind organization. I'd say you should focus on adding the MSG2_STREAM_UNREGISTER now and then do a full-scale overhaul of the networking. You can do it.

I won't merge the... It's only there to test...

OK, I see your work pattern now. I'll just let you code and stop drawing premature conclusions. However, I suggest you mention "this is WIP" in the PRs so this is clear.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 19, 2016

Contributor

#936 (comment)

Work in progress:

TODO: Implement MSG2_STREAM_UNREGISTER messages

bugfixing and refactoring at the same time doesn't really work

Yeah, I think I should really stop doing that. 😊

I'd say you should focus on adding the MSG2_STREAM_UNREGISTER now and then do a full-scale overhaul of the networking.

Agreed.

Contributor

ulteq commented May 19, 2016

#936 (comment)

Work in progress:

TODO: Implement MSG2_STREAM_UNREGISTER messages

bugfixing and refactoring at the same time doesn't really work

Yeah, I think I should really stop doing that. 😊

I'd say you should focus on adding the MSG2_STREAM_UNREGISTER now and then do a full-scale overhaul of the networking.

Agreed.

@ulteq ulteq merged commit ed7fcdb into RigsOfRods:master May 19, 2016

1 check passed

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

@ulteq ulteq deleted the ulteq:small-network-cleanup branch May 19, 2016

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