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

Removing duplicate websocket sharp #6177

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

ProteanCoding
Copy link

@ProteanCoding ProteanCoding commented Apr 18, 2024

What does this PR change?

This is to fix code duplication.
This code has been maintained here: 9ad1866
But a different version has been used here: https://github.com/decentraland/unity-renderer/blob/dev/.circleci/config.yml#L18

@aixaCode
Copy link
Collaborator

Are you sure this is correct? If you remove websocket-sharp-master from unity-renderer/Assets/Plugins, Unity project will not compile as there is code that references it.
Also I don't see anything wrong with the project path, it's working properly for all platforms. I think we can remove the unity-renderer/Plugins one.
Also Examples seems to be safe to remove.

@ProteanCoding ProteanCoding marked this pull request as draft April 19, 2024 14:22
@ProteanCoding
Copy link
Author

ProteanCoding commented Apr 19, 2024

Sorry I meant to keep this as Draft, I was getting overwhelmed with all the PRs on my plate, so I decided to post all the others, and pull this one out of #6173, so that that one at least could stay ready to merge.

@ProteanCoding
Copy link
Author

I am indeed not sure this is correct.
But any code duplication is in itself inherently incorrect, and really should be addressed. It makes it dangerously hard to ensure that further modifications are properly propagated.
I will create an independent Issue for this.

@aixaCode
Copy link
Collaborator

Unfortunately, you can't run our automated checks as an external contributor, and I would need to move your branches to our repo so the CI will pick it up. I'm a bit lost with all the PRs you have opened ;)

Regarding the duplicate WebSocket Sharp library, I'm sure the project will not compile. Just to confirm, I pulled your branch and opened it in Unity, and there are quite a few compiler errors. Also, our CI has failed with compilation errors.

Screenshot 2024-04-19 161203

As a suggestion, it would be good to test your changes locally before opening a PR to catch issues like this.

@ProteanCoding
Copy link
Author

Yes as I wrote, I meant to keep this one in Draft. All my other PRs have been properly tested.

@ProteanCoding
Copy link
Author

ProteanCoding commented Apr 22, 2024

So I've been reviewing the train of thought regarding my original attempt at removal of the whole unity-renderer folder.

For make build-unity-local in the browser-interface's Makefile, @dcl/explorer is simply copied out of node_modules.

cp node_modules/\@dcl/explorer/unity* static/

Thus I assumed the unity-explorer project was pulled in at that point, and that files remaining in the unity-renderer folder were left-over from a transfer to that project.

I now see that this in fact "pulls the result of the unity-renderer folder", which is what is being published via CircleCI onto https://www.npmjs.com/package/@dcl/explorer.

Talk about misnomers !!

@ProteanCoding
Copy link
Author

ProteanCoding commented Apr 22, 2024

I hadn't even compiled that folder at that point, as it needs Unity 2022.3.6f1, and the unity-explorer project itself needs Unity 2022.3.23f1 (part of why I thought this folder was stale).
A Unity engine is a hefty download 😖

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