search

pplans / foundry-vtt-module-motion-tracker

A module giving life to the Alien RPG motion tracker in foundry VTT
MIT License
6 stars 8 forks source link

Fix for v11 #58

Closed CodaBool closed 1 year ago

CodaBool commented 1 year ago

🚩 Changes

🔉 Audio

There was a sound board object but it seems the idiomatic foundry method for sound is through the AudioHelper. I've switched over to that. This resolved a warning of "audio not preloaded". I don't attempt to do any preloading when using the new AudioHelper. So, I'm not sure how this compares on performance but I don't notice any issues.

File changes

I replaced the default audio files. I found them to be abrasive but is not essential for the PR since they can be swapped by the client. Give them a listen and let me know which you prefer and I can have the PR match that.

https://github.com/pplans/foundry-vtt-module-motion-tracker/assets/61724833/b461e6e9-8748-4516-a692-21232fdc2daa

https://github.com/pplans/foundry-vtt-module-motion-tracker/assets/61724833/7fd6ccc5-cebd-434b-8aa2-f0aa59697a05

https://github.com/pplans/foundry-vtt-module-motion-tracker/assets/61724833/4a7a6d43-a92c-4fa7-b9d8-d42b5ee733cf

https://github.com/pplans/foundry-vtt-module-motion-tracker/assets/61724833/d93db89c-afca-476d-8b20-4aa086ef58a1

👤 Status

There was an error with getting the status effects. I resolved it and will return early for if the effect is not found. This should improve performance.

🖼️ Textures

PIXI docs stated that loading a texture multiple times does not matter. So, I removed a conditional that checks if the textures are loaded. I put a console.log in there and it's not ran really more than once so I don't think it was necessary.

There was an error about using a PIXI loader. This was because the code seems to assume a PIXI version of 5 but it should use methods for version 7. There is even an exact example in their migration docs which says how textures should now be loaded. With that change I was able to simplify how textures got loaded.

🧪 Tested

I've tested it locally on Chrome with 2 browsers (one incognito) and wasn't getting any errors. Everything seems to work just fine.

pplans commented 1 year ago

am reviewing the rest tomorrow, sorry am usually kinda busy but I have this in mind very well no worries

pplans commented 1 year ago

am very unsure about the audio, they really don't sound like the ones in the movies or video games :/ can we split this into two PRs ? The fix for v11 and the audio files ?

CodaBool commented 1 year ago

that's totally fine, your module has a way to upload a audio files so I have no problem at all removing that. I'll update the PR later today.

pplans commented 1 year ago

thanks man, sorry about the delays, been a bit sick and had a huge work week so far, I'll probably validate the v11 PR eyes close (right away :))

CodaBool commented 1 year ago

ok, the diff is straightened up. I can run a test of it and see how that goes.

CodaBool commented 1 year ago

Was having an issue of it still trying to use the old path for the audio file. However, I think that was just because there is some kind of Foundry cache on settings and that overwrite any defaults in the current build.

Once I changed the settings to their defaults I was getting the original sounds. I think this one is all in order. I am going to bump the version and mark it as working in V11 in the module.json and then it should be all good.

CodaBool commented 1 year ago

🚀

Txus5012 commented 1 year ago

Hi. Thanks for your work on this.

You should take a look at the deprecation warnings. Warnings

Also, in v11 the compatibility field should look like this or another deprecation warning will appear: Compatibility

pplans commented 1 year ago

I didn't notice, but can you PR this into the dev branch ? so that we can prepare an official good update maybe we can pull something out compatible for both 10 & 11. @Txus5012 can't this be set to 10 as well ? there's probably a way

Txus5012 commented 1 year ago

Do you mean the compatibility field? You can write it in both old and new methods and will work for 10 & 11. But I don't know if you can make it work for both versions of PIXI. Anyway, people using v10 can still download and use the older version of the module.

CodaBool commented 1 year ago

I'm still new to foundry development so building out that compatibility would take me a lot of time. I'm going to be busy the next weeks and won't have time to assist efforts to make it both v10 & v11 compatible.

pplans commented 1 year ago

yeah it's probably too much work and not worth enough, can we make this PR go to the dev branch tho ? Am not very proficient on git since I don't use this professionally. All the dev is going to the dev branch and I merge stable versions to main, or if a patch has too much priority gets his own dev -> main.

If it's too much constraints I'll merge on main and move back to dev but it's kind of a mess sometimes, next PRs should go on dev branch

CodaBool commented 1 year ago

Sure ill make a new PR against dev

pplans commented 1 year ago

thanks Coda! Am installing v11, should have done that a while ago but I got very busy elsewhere, I'll try to get it done in the worst case in 10 hours from now (gotta sleep at some point)

pplans commented 1 year ago

PR has been merged into dev branch, will soon be published, thanks Coda for your work