TyphonJS•2y ago

Actor update error, TRL/Svelte Token interaction?

Ran into a bug I think, not sure if this is with Foundry or TRL. Related to the duplicate actor sheet issue Geoidesic ran into a few months back. Same issue, I'm getting duplicate character sheets when I try to run

actor.update()

actor.update()

. I traced the error back, looks like it's coming from

TokenDocument._onUpdateBaseActor()

TokenDocument._onUpdateBaseActor()

making a

this.actor.sheet.render(false)

this.actor.sheet.render(false)

call on line 565 in Foundry client/data/documents/token.js. I think this call to render is unnecessary for TRL and is causing SvelteApplication.js to throw a cannot read properties of null error on line 876 at

this.onSvelteMount({ element: this._element[0]...

this.onSvelteMount({ element: this._element[0]...

, _element[0] is undefined in this case. Looks like

TokenDocument._onUpdatedBaseActor()

TokenDocument._onUpdatedBaseActor()

isn't respecting the

{render: false}

{render: false}

context property, so passing render: false doesn't fix the issue. Not sure if that's intended behavior or not. Right now I'm looking into extending TokenDocument and just removing that line from the override as a workaround, but that's territory that I haven't really been involved in up to this point so I'm not 100% sure how to go about it. Any help would be greatly appreciated. :) You can check out my code here: https://github.com/Lekyaira/itoa/tree/19-re-format-side-bar The offending call starts here on line 19: https://github.com/Lekyaira/itoa/blob/19-re-format-side-bar/src/sheets/actor/Sidebar.svelte

GitHub

GitHub - Lekyaira/itoa at 19-re-format-side-bar

Contribute to Lekyaira/itoa development by creating an account on GitHub.

GitHub

itoa/Sidebar.svelte at 19-re-format-side-bar · Lekyaira/itoa

Contribute to Lekyaira/itoa development by creating an account on GitHub.

26 Replies

RyanWAndersonOP•2y ago

Nekro Darkmoon•2y ago

this.actor.sheet.render(false) doesn't actually say to not render the first parameter of that function is force all you're saying is to not force it you want to get rid to the entire line 😄

TyphonJS (Michael)•2y ago

Let me know if the suggestion from ND above is a solution. TRL / SvelteApplication guards against subsequent calls to render, but this is a strange occurrence seemingly on an unintended first render. I could adjust the onSvelteMount access to this._element[0] which is just getting that actual HTMLElement from the Foundry JQuery reference. IE change it to this._element?.[0], but that is just masking the actual problem. A second sheet / unintended render would still occur. You can go in and locally make that mod and see what happens, but I gather it'll still be a bit borked. If you do have more debugging info please do share it here. I'm pretty deep in another area currently, so don't want to do a context switch this week if possible. IE track down the bug. However, if it is a problem in Foundry now is the best time to get a suggestion in through the mothership to get it fixed in v11. It wouldn't hurt either to test on v11 as there were changes to tokens / "synthetic actors" or something like that.

Nekro Darkmoon•2y ago

so the second sheet rendering is actually the token sheet there's an entire thing with unlinked tokens and stuff that is super annoying hopefully it gets better with v11

TyphonJS (Michael)•2y ago

Yeah, I'm only vaguely aware that v11 makes some significant changes there hence testing on v11 is probably a good idea.

Nekro Darkmoon•2y ago

yup there's also a thing where a linked actor sheet can't tell if its been opened by a token or from the sidebar These lines are the fix for that https://github.com/Pjb518/FoundryVTT-Level-Up-Official/blob/main/src/apps/ActorSheet.js#L64 https://github.com/Pjb518/FoundryVTT-Level-Up-Official/blob/main/src/apps/ActorSheet.js#L70 https://github.com/Pjb518/FoundryVTT-Level-Up-Official/blob/main/src/apps/ActorSheet.js#L74-L76 https://github.com/Pjb518/FoundryVTT-Level-Up-Official/blob/main/src/apps/ActorSheet.js#L353-L357 essentially a problem that arises without the above fix is that token header icons for linked actors always show the prototype token settings instead of the token settings. 😄 I believe the last time I checked @solidor system suffered from the same issue as well

TyphonJS (Michael)•2y ago

Do keep me informed if there is anything actionable on the TRL side of things. From what I can tell it's core Foundry weirdness somewhere in the chain.

Nekro Darkmoon•2y ago

yup it somehow populates the _sheet property via a hook somewhere

RyanWAndersonOP•2y ago

That's what I suspected was going on. Thanks for the info! Hopefully v11 will fix it. Until then, it's workaround time! Haven't had time to look at this again until now. I implemented the changes @nekrodarkmoon highlighted above, didn't seem to affect the issue. Going to try overriding token and see what that breaks now.

TyphonJS (Michael)•2y ago

Are you able to test in v11? I think that is probably the best environment to do any testing in presently. If there are additional problems w/ v11 quite possibly they can be addressed now before v11 ships.

RyanWAndersonOP•2y ago

I don't have it set up. I'll take a look at it.

TyphonJS (Michael)•2y ago

At least for modules there is nothing really standing in the way of running on v11; hopefully the same more or less for systems. Though the DB swap is the biggest sticking point probably. Are you using the Node distributions of Foundry? That is the easiest way to set things up. I have v9, v10, v11 setup side by side and can easily switch between them. You can also make symlinks between them IE in the Data/modules or presumably even Data/systems can make symlinks to the v10 Data/modules or systems directory and it works fine. Not sure actually how that would affect packs though. I've run FQL on v11 w/ the symlink and it has packs. I haven't looked too deep into the DB swap angle. In the next FQL release I'm rewriting things to use the legacy mode of TRL and removing the packs in favor of a unified settings menu built into FQL and moving all of the packs which were for macros into the unified settings menu.

RyanWAndersonOP•2y ago

Oh wow it's all different looking now lol Issue persists. The functions are written differently now though. It's under

if ( !this.isLinked && this.delta ) {
      this.delta.updateSyntheticActor();
      this.actor.sheet.render(false);
    }

if ( !this.isLinked && this.delta ) {
      this.delta.updateSyntheticActor();
      this.actor.sheet.render(false);
    }

Makes me think this is intended behavior, might should be caught under

actor.sheet.render

actor.sheet.render

instead of at the token

_onUpdateBaseActor

_onUpdateBaseActor

They're trying to call an update of the displayed sheet data, we're the ones who are hooking that behavior.

TyphonJS (Michael)•2y ago

I don't have much visibility into the system angle w/ demo code to work on directly. I think it's unfortunate that they don't pass along a context about why the sheet is being rendered. This is one thing that can absolutely be brought up in the v11 dev or dev-support channels on the main Foundry Discord server right now. In ClientDocumentMixin a fair amount of the render calls from Foundry include a context object that give the reason why the render was invoked. For instance look at ClientDocumentMixin._onUpdate. It invokes the associated apps render method with a context object:

this.render(false, {
    action: "update",
    data: data
});

this.render(false, {
    action: "update",
    data: data
});

You should really make a post now on the mothership asking the devs to make that render call contain a context object with action specified with perhaps updateSyntheticActor like this:

this.actor.sheet.render(false, { action: "updateSyntheticActor" });

this.actor.sheet.render(false, { action: "updateSyntheticActor" });

That way it is trivial to override render in the associated SvelteApplication and at least be able to specifically respond if necessary to this particular render call if any workarounds need to be implemented. --- FYI the render context data is the only reason TJSDocument works reactively. TJSDocument registers with ClientDocumentMixin apps and fakes the app API called (render / close). The crud updates w/ context to render allow TJSDocument to function. The Foundry document model is rather weak when it comes to a proper event model and the current app registering is a kludge. ----------- Come to think of it I can add into SvelteApplication an easy to use "renderHandler" capability where in SvelteApplication I provide the render override and when a context option with "action" key is in the object I invoke a registered "renderHandler" with the context object and if the renderHandler returns a "falsey" value the render is cancelled automatically. I'd probably put that under SvelteApplication.reactive.renderHandler where you could in the constructor do something like this:

this.reactive.renderHandler = ({ action }) => {
   return action !== 'updateSyntheticActor';
}

this.reactive.renderHandler = ({ action }) => {
   return action !== 'updateSyntheticActor';
}

This would return false if the core Foundry team adds the context like I mentioned above and cause the render to not occur.

RyanWAndersonOP•2y ago

Cool, I'll throw that their way and see what happens.

TyphonJS (Michael)•2y ago

Feel free to chime in... I just made a post: https://discord.com/channels/170995199584108546/1065764029278212230/1104120148790607942

RyanWAndersonOP•2y ago

Awesome, thanks!

TyphonJS (Michael)•2y ago

You might actually want to chime in on that message as it's hard to say how much my suggested advice will get listened to by the core team. Perhaps @nekrodarkmoon too. Having a concrete use case for why adding the context object to all core invocations of Application.render otherwise might not be taken as a pressing concern. It's really a relatively simple change, but the core team may not realize the impact it can have especially for the updateSyntheticActors use case to be able to easily tell when a render is invoked in that case. I updated my post about at least covering the "passive rendering" use cases which there are far fewer, but this does cover the updateSyntheticActors use case. Well we tried... Just hope and prepare for disappointment.. ;P About Foundry standard.. ;P The whole render / context object thing is already way fractured w/ little consistency. The ClientDocumentMixin callbacks for direct documents use "create", delete", update" under an action attribute, but embedded documents context objects use renderContext instead of action. On v11 the renderContext strings have changed with an additional . separator. And that is just the start; no consistency. The whole it's too big of an add to add a context object even for updateSyntheticActor is more a symptom of the core team not knowing the scope of what exists in general. I'm not sure what can be done to at least convince them that this one context object aspect is not going to cause a problem and solve a tough problem as it seems the issue persists on v11. The App v1 API has always been a weak link IMHO.... At least they fixed some issues I reported for a couple of areas like the Dialog.wait implementation. Fingers crossed, but don't get your hopes up.

RyanWAndersonOP•2y ago

Yup. I'm not sure why token treats its actor sheets separately to begin with. Actor sheet should be the actor sheet, token should be a token. I suspect it's because of the prototype/token difference. Maybe because of NPCs where you want different data for multiple duplicate tokens... I'm also getting some weird desync between player token and actor sheet data. I haven't spent the time to track that down yet but I suspect it's related.

TyphonJS (Michael)•2y ago

It's a minor miracle I got TJSDocument / TJSDocumentCollection to work as there isn't a real event system; though I think I already brought that up. IMHO ClientDocumentMixin and DocumentCollections would benefit from a very simple subscribe capability just like the bare bones Svelte readable store contract. A single method that returns an unsubscribe function. No silly business with faking an "app" and inserting oneself. That whole idea / implementation is a hold over from the very beginnings of Foundry where it was a quick hack that snowballed. IMHO the App v1 infrastructure is so pervasive across so many areas. It's an intractable situation and I don't think the core team knows how to dig out of that hole. Hopefully, I can work with them a bit in the future; nothing Svelte related, but kind of in this holding period. Wait and see 1 or 2 more core versions.

RyanWAndersonOP•2y ago

Aaaaaand it was that easy! I made a TokenDocument class that passed options to the render function:

    /**
     * When the base Actor for a TokenDocument changes, we may need to update its Actor instance
     * @param {object} update
     * @param {object} options
     * @internal
     */
    _onUpdateBaseActor(update={}, options={}) {

        // Update synthetic Actor data
        if ( !this.isLinked && this.delta ) {
            this.delta.updateSyntheticActor();
            
// This is the only change lol
this.actor.sheet.render(false, options);
        }

        this._onRelatedUpdate(update, options);
    }

    /**
     * When the base Actor for a TokenDocument changes, we may need to update its Actor instance
     * @param {object} update
     * @param {object} options
     * @internal
     */
    _onUpdateBaseActor(update={}, options={}) {

        // Update synthetic Actor data
        if ( !this.isLinked && this.delta ) {
            this.delta.updateSyntheticActor();
            
// This is the only change lol
this.actor.sheet.render(false, options);
        }

        this._onRelatedUpdate(update, options);
    }

And in my function that updates the actor I passed options.action = 'heropointClick'; then in the ActorSheet's render function I just checked for that action and returned if I found it. Error fixed! Everything works as intended. Now if only we can get this change pushed to live. -_-

TyphonJS (Michael)•2y ago

You should really post this as a follow up. As I mentioned they could easily change that code to this:

this.actor.sheet.render(false, { action: 'updateSyntheticActor', options: { ...options } });

this.actor.sheet.render(false, { action: 'updateSyntheticActor', options: { ...options } });

Do you modify the options data in the render method? That is the reason I made a shallow copy of it above so that it loosely is not modified.

RyanWAndersonOP•2y ago

Will do

TyphonJS (Michael)•2y ago

So do you really need the options object or do you just need the action: 'updateSyntheticActor' part? The way Atro responded took the negative towards passing on the options. Even if options wasn't passed on would this suffice?

this.actor.sheet.render(false, { action: 'updateSyntheticActor' });

this.actor.sheet.render(false, { action: 'updateSyntheticActor' });

It is being dismissed for the wrong reason per se.

RyanWAndersonOP•2y ago

Yes that would work just fine. I just need to know where the render call is coming from.

TyphonJS (Michael)•2y ago

You should communicate that as it is being dismissed for passing the options object through. IE that muddied the water. There is no reason a context object can't be added that simply has the action attribute or much less of a reason not to. Oooo Ooooo... Maybe.... maybe... Heh: referring to recent discussion in the v11 feedback channel on the mothership.. ;P @ryanwanderson you might want to chime in there and confirm. I think Atro is open to changing it to:

this.actor.sheet.render(false, { renderContext: 'updateActor' });

this.actor.sheet.render(false, { renderContext: 'updateActor' });

And you can inspect this.actor to determine if it is a synthetic actor.

Gaming

Programming

Actor update error, TRL/Svelte Token interaction?