League-of-Foundry-Developers / torch

Simple torch module for Foundry VTT
10 stars 16 forks source link

Additional light source named "Dancing Lights" in non-DnD5e systems broken #40

Closed si-lang closed 1 year ago

si-lang commented 1 year ago

Foundry version: 10 (291) System: Pathfinder 1 (0.82.5) Module version: 2.4.0

Additional Light Sources file:

{
    "pf1": {
        "system": "pf1",
        "topology": "standard",
        "quantity" : "quantity",
        "aliases": {
            "Dancing Lights": "Light"
        },
        "sources": {
            "Light": {
                "name": "Light",
                "type": "spell",
                "consumable": false,
                "states": 2,
                "light": [
                { 
                    "bright": 20, "dim": 40, "angle": 360, "color": "#001010", "alpha": 0.5, "luminosity": 0.5,
                    "animation": { "type": "pulse", "speed": 3, "intensity": 1, "reverse": false }
                }
                ]
            }
        }
    }
}

Hello and happy new year everybody!

In my Pathfinder 1 game I use your very nice module to enable my players toggling their light state more easily. Your module already includes several items which can be used as a light source in pf1, but I found, that there where no spells included. This is why I created an "Additional Light Sources" file. This file currently only includes one light source "Light", which is a spell and one alias "Dancing Lights", which should behave similarly to "Light" in my game for simplicity reasons. Everything works as expected to this point:

But nothing happens when the button is clicked. The console prints the following error message and stacktrace:

Uncaught (in promise) Error: This is not a registered game setting
[Detected 1 package: torch]
    at ClientSettings.get (foundry.js:2799:42)
    at Object.createDancingLights [as create:Dancing Lights] (request.js:37:27)
    at TorchRequest.perform (request.js:31:38)
    at TorchSocket.handleSocketRequest (socket.js:12:28)
    at TorchSocket.sendRequest (socket.js:42:21)
    at TorchToken._turnOnSource (token.js:140:19)
    at TorchToken.advanceState (token.js:108:20)
    at async toggleLightSource (torch.js:57:20)
    at async HTMLElement.<anonymous> (hud.js:56:13)

The underlying problem in the module's code base is the following: When executing requestSupported() in token.js line 138, somewhen in the call hierarchy supports() in request.js line 27 is called. Line 28 in this file checks the contents of a static array for equality and returns true, since 'Dancing Lights' is contained: return requestType in TorchRequest.ACTIONS; Therefore, the call in token.js line 138 evaluates to true.

After this, the module just assumes, that "Dancing Lights" is supported in a DnD5e fashion (I did not dive any further here what that exactly means) and tries to let v = game.settings.get("torch", "dancingLightVision"); in request.js line 37. This line fails, since settings.js only registers the requested setting if the system is DnD5e:

if (game.system.id === "dnd5e") {
  game.settings.register("torch", "dancingLightVision", {
    name: game.i18n.localize("torch.dancingLightVision.name"),
    hint: game.i18n.localize("torch.dancingLightVision.hint"),
    scope: "world",
    config: true,
    default: false,
    type: Boolean,
  });
}

I would propose adding an additional check for the currently active system in the requestSupported() flow as a potential fix. This would also fix the workflow when turning the light source off again (same issue, just beginning in token.js line 123 instead). Thank you for this great module and keep up the good work!

Kind Regards, DerGote

Edit: Formatting Edit2: Additional hint

lupestro commented 1 year ago

Thanks. I'll put this on a short list to clean up - and thanks for looking at the code. Since you're at least 80% of the way there, do you want to submit a PR?

lupestro commented 1 year ago

@si-lang I've just issued release 2.6.0 with code that should fix this. Please test.

lupestro commented 1 year ago

Hearing nothing to the contrary, I’m going to close this issue.

si-lang commented 1 year ago

@lupestro Thank you for the update! Your fix works for me. Keep up the good work!