Skip to content

Communicator factory #4965

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

Merged
merged 1 commit into from
Feb 19, 2021
Merged

Communicator factory #4965

merged 1 commit into from
Feb 19, 2021

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Feb 18, 2021

Proposed change(s)

Follow up for the "RpcCommunicator on non-desktop" compilation problems.

  • Makes all of RpcCommunicator.cs empty on non-desktop.
  • Adds a CommunicatorFactory that returns either an RpcCommunicator or null
  • Cleans up RpcCommunicator - it was being passed CommunicatorInitParameters in both it's constructor and Initialize() method. It stored the former but only used the port from it.
  • (possibly controversial) Setting CommunicatorFactory.Enabled = false will skip creating (and attempting to connect) with RpcCommunicator. This is a bit clunky but it's the only way we have to avoid trying to connect to a trainer, which could be a problem for shipping games.

Types of change(s)

  • Bug fix
  • Code refactor

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@chriselion chriselion requested a review from surfnerd February 18, 2021 03:51

/// <summary>
/// Initializes a new instance of the RPCCommunicator class.
/// </summary>
/// <param name="communicatorInitParameters">Communicator parameters.</param>
public RpcCommunicator(CommunicatorInitParameters communicatorInitParameters)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't pass these and don't store them anymore. They were only used for the port; that's now passed to Initialize()

Copy link
Contributor

@surfnerd surfnerd left a comment

Choose a reason for hiding this comment

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

🙏

Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

How did you test this was working for mobile builds ?

@chriselion
Copy link
Contributor Author

chriselion commented Feb 19, 2021

Manual tested that I can do a WebGL build locally.

I have a PR to start doing a WebGL build in yamato to guard against similar compiler errors in the future: #4966 (currently failing because it lacks this fix)

@chriselion chriselion merged commit e5df4a2 into master Feb 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the communicator-factory branch February 19, 2021 04:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants