Skip to content
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

Multiplayer! #1431

Merged
merged 17 commits into from
Aug 19, 2021
Merged

Multiplayer! #1431

merged 17 commits into from
Aug 19, 2021

Conversation

AGoblinKing
Copy link
Contributor

@AGoblinKing AGoblinKing commented Aug 19, 2021

  • Enables integration with github.com/webaverse/dialog for multiplayer poses
  • Uses the eslint-prettier autoformatter, let me know if you don't like any of the styles and we can update the options

biz-gif

Depends on webaverse/dialog#7

@AGoblinKing AGoblinKing requested a review from avaer August 19, 2021 17:08
@AGoblinKing AGoblinKing mentioned this pull request Aug 19, 2021
3 tasks
@AGoblinKing AGoblinKing marked this pull request as draft August 19, 2021 17:52
@AGoblinKing
Copy link
Contributor Author

Well back to draft, trying out a better way of doing the hashHost passing

@AGoblinKing AGoblinKing marked this pull request as ready for review August 19, 2021 17:56
@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

Uses the eslint-prettier autoformatter, let me know if you don't like any of the styles and we can update the options
I don't think it's consistent with the ESLint rules, but also: I don't think this is the PR to do it.

We should do a full pass over to code to pretty it to either eslint or prettier, but we shouldn't keep multiple formats and format types, and we shouldn't make that change in an unrelated PR.

@AGoblinKing
Copy link
Contributor Author

I can take the linter out and undo any of its funkier changes. This should be enforcing the existing style guide though, so no file should change if it meets the lint requirements.

@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

This should be enforcing the existing style guide though

Ah you're right, I'll clean up the eslint rules because they are not quite what I think we've been using for the code.

@AGoblinKing
Copy link
Contributor Author

AGoblinKing commented Aug 19, 2021

I took prettier out. Should I revert those style changes to the files touched or keep them? My linter is mad at me with the old stuff per your reply.

image

@AGoblinKing
Copy link
Contributor Author

image
for comparison

@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

I took prettier out. Should I revert those style changes to the files touched or keep them? My linter is mad at me with the old stuff per your reply.

Yeah we should fix that but not in this issue.

@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

One particularly nasty rule is the casing of hex literals, which are usually pasted from other tools and lists that we do not want munged by the prettier.

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/number-literal-case.md fails to mention what option to enable and to what value.

@AGoblinKing
Copy link
Contributor Author

I use .prettierignore to avoid munging third party libraries. Lets talk about the style in another CL. I can revert the significant style changes here easily.

@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

I use .prettierignore to avoid munging third party libraries.

It's not the libraries, it's the content we paste in to local source files from sources like design templates (eg constantly changing
massive colors lists)

@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

I updated the .eslintrc in master to reduce the amount of code changes next time we run it.

@AGoblinKing AGoblinKing marked this pull request as draft August 19, 2021 19:21
@AGoblinKing AGoblinKing marked this pull request as ready for review August 19, 2021 19:36
@AGoblinKing
Copy link
Contributor Author

Okay that should be easier to read, minus RoomClient which greatly needed some formating.

constants.js Outdated Show resolved Hide resolved
constants.js Outdated
export const dialogUrl = isGH
? isOKHASH
? hashHost[2]
: window.location.hostname.replace('3000', '4443')
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better using a regex. We don't want to corrupt domains like robot3000.club.

Copy link
Contributor Author

@AGoblinKing AGoblinKing Aug 19, 2021

Choose a reason for hiding this comment

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

it could affect the githubpreview.dev hostname including subdomains only which seems unlikely. We're relying on an undocumented naming convention is the bigger issue imo.

constants.js Outdated Show resolved Hide resolved
client/RoomClient.js Outdated Show resolved Hide resolved
world.js Show resolved Hide resolved
constants.js Outdated Show resolved Hide resolved
@avaer
Copy link
Contributor

avaer commented Aug 19, 2021

Keep in mind that we lose hosted rooms support with this change:

https://github.com/webaverse/app/pull/1431/files#diff-7aecd33c076eede220000d2edb73e82e0230c5fcb781bb22f8831b7e37b01b77L246

However, we can/should bring that back once we have audio working again.

@avaer avaer merged commit 439a854 into webaverse:master Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants