Blockade-Games / BlockadeLabs-SDK-Unity

44 stars 16 forks source link

Update package deps #1

Closed StephenHodgson closed 1 year ago

goran-popovic commented 1 year ago

Hi there,

Thanks for the PR, and sorry for a bit late response. This is something I tried before, it would be very helpful, but unfortunately it won't work, probably because it's not referencing the UPM branch of the package, it's just trowing an error: https://www.screencast.com/t/IIPyTOtI

StephenHodgson commented 1 year ago

@GoranPopovic1 what is your method of installation?

I'm using OpenUPM which manages the deps automatically. If you're using the git installer, you need to manually install the dep first.

StephenHodgson commented 1 year ago

I had to recently add the pusher package to OpenUPM to get this to work correctly for me, which is why I am opening this PR bc it is not correctly installing deps as they are not referenced properly in the package info.

goran-popovic commented 1 year ago

@StephenHodgson I'm using the methods mentioned here: https://github.com/Blockade-Games/BlockadeLabs-SDK-Unity#install

So either from GitHub URL in the package manager or adding the URL directly in the manifest.json, and since the Pusher is an optional dependency, it might be better if it isn't referenced in the package.json as a dependency. Even if we did reference it and add it to package.json, those errors would still be trown for users not using OpenUPM.

But feel free to fork the project, like you did, and make the necessary adjustments that fit your needs. This project just serves as an example of using our API, and you can build your own solution using it as a starting point.

StephenHodgson commented 1 year ago

I don't understand how pusher is optional. It's required to get events.

I may just do that and I'll ping scottie, since I think I've been asked to also implement the Unreal plugin as well.

goran-popovic commented 1 year ago

Pusher is optional since API data pooling is implemented by default, when generating assets in the Editor and on Runtime. When using the Pusher package events are being used on Runtime instead of the data pooling. Also seems that the Pusher package might have issues with WebGL builds: https://github.com/pusher/pusher-websocket-unity#unsupported-platforms. So we decided at that point to leave the choice to the users.

I wasn't aware that Julien added the package to OpenUPM: https://openupm.com/packages/com.blockadelabs.sdk/ and since you added the Pusher package there as well (thanks for that, appreciate it :)), now users can install both of the packages through OpenUPM if needed. I've tried installing the packages using that approach now, and everything seems to be working fine, so I've updated the README file: https://github.com/Blockade-Games/BlockadeLabs-SDK-Unity#use-openupm-cli

StephenHodgson commented 1 year ago

I think it's safe to assume most people building for the web will just use web dev tools instead of Unity. It's a bit of overkill and additional complexity when just using the js versions of the sdk might just be the easiest thing to do.

StephenHodgson commented 1 year ago

Adding the package dep will simplify installation process for those using OpenUPM, which imho should be the suggested/default way to install the package, as git installation method is prone to breaking, and makes versioning more difficult.

goran-popovic commented 1 year ago

I agree that would simplify the installation process for the users using OpenUPM, but users using other two methods would still get errors if we add that dependency. Which might raise more issues on the Discord where we are supporting those users mainly.

We have a variety of users with different backgrounds, some of which are using Unity for the first time, and I would like to keep all 3 ways of installing the packages on the table, without any issues, at least without the ones we can sort out beforehand.

OpenUPM, to me, seems to be more for seasoned developers, since you have to install npm and run some commands in the terminal, and those developers shouldn't have much additional work before them, now that you submitted the Pusher package as well to OpenUPM. For an average user, it might be easier to just paste the link inside the Unity editor.

I'm open to discuss this next week further if needed, I still don't know in which capacity you will be working with us, but like I said, we can address this further next week, with my superior, with Scottie, and anyone else involved. We can give you access to this repo if needed, or you could create a fork which might go in some other direction.

Have a nice weekend.

StephenHodgson commented 1 year ago

OpenUPM, to me, seems to be more for seasoned developers, since you have to install npm and run some commands in the terminal

This is not true. You only need to add the scoped registry in the unity editor project settings.

StephenHodgson commented 1 year ago

Out of all 3 methods OpenUPM is the easiest and simplest of the installation methods.

goran-popovic commented 1 year ago

This is not true. You only need to add the scoped registry in the unity editor project settings.

You are right, but I also see that OpenUPM recommended using a command-line tool: https://openupm.com/docs/#scoped-registry-and-command-line-interface

And I still think that for the average user it will be easier to grasp the concept of adding a GitHub URL than the concept of scoped registries.

Also, the users who want to use OpenUPM can do it now nevertheless, thanks to Julien and you, so it's just a matter of preference at this point.

Anyway we have started to drift a bit from the original PR's purpose, I'm going to close it now, but like I mentioned, feel free to fork the repo if you think it should go in another direction.

Have a nice day.

StephenHodgson commented 1 year ago

feel free to fork the repo if you think it should go in another direction.

I really don't understand why you keep suggesting this, but I suppose I will 😏

StephenHodgson commented 1 year ago

well your wish came true: https://github.com/RageAgainstThePixel/com.rest.blockadelabs/releases/tag/1.0.0