Advancement Hooks

[aaclayton]

Originally in GitLab by @akrigline

We should consider adding some hooks that allow modules to inject themselves into the advancement process.

There are several modules today which interact with the levelup process and providing a standardized framework for them to use instead of hack around our solutions would be ideal.

Given the preliminary generalized flow of: (See #1357 for a better understanding of the UI flow as we make decisions)

1. User Levels up a Class on an Actor OR adds a new advancement item to an actor
2. Advancement options are presented to that user based on the actor's (new or old) level

• Each level of the actor is a 'step' in the prompt

3. User's Actor is updated with choices

What would be the most useful set of hooks for module authors? I'm going to ping @kandashi and @iaguastalli as they are the two module authors I know of off-hand who are interacting with this api space.

I'll add some comments with proposed hooks.

[aaclayton]

Originally in GitLab by @akrigline

Proposal:

Hooks.call('preAdvancementDialog')
// args:
// - Actor

Or "PreCharacterAdvancement". Fires after step 1 but before step 2. Would allow a module to cancel the advancement dialog's creation entirely.

[aaclayton]

Originally in GitLab by @akrigline

Hooks.callAll('advancementStepRender')
// args:
// - advancement data for this step of the prompt
// - html being rendered
// - Synthetic Actor represented by previous advancement decisions?

Fires at the end of each step's render in the prompt Application. Would allow a module to add to or modify the output html for this step (potentially adding more data to the step).

[aaclayton]

Originally in GitLab by @akrigline

Hooks.callAll('advancementStepComplete')
// args:
// - advancment step form data for this step of the prompt
// - Synthetic Actor represented by previous advancement decisions?

Fires after the user makes selections in a given dialog step but before the synthetic actor is updated. Would allow modules to change these mutations to the synthetic actor (e.g. to include things from their own UI elements)

The final actor update would go through the normal document hooks so a specific one for Advancement is probably unnecessary.

[aaclayton]

Originally in GitLab by @kandashi

I think those suggested hooks would perfectly fit with what I need

[aaclayton]

Originally in GitLab by @iaguastalli

I think Kandashi would makd better use of this as my module's advancement is still very much WIP, but I agree with him that this should cover our bases.

[aaclayton]

Originally in GitLab by @Fyorl

With regards to step 1, it seems like those two things are quite different from each other and should not be handled by the same hooks. For the levelling up process, I think the suggested hooks make sense, subject to further iteration. For the workflow around adding, updating, and deleting advancement objects themselves, I'd propose we implement similar CRUD hooks to core that should be familiar to devs by now, e.g. dnd5e.preCreateAdvancement, dnd5e.createAdvancement, dnd5e.preUpdateAdvancement, etc.

[aaclayton]

Originally in GitLab by @arbron

@akrigline Now that the core of the advancement system is in place for the most part, it might be a good time to revisit these hooks based on what we ended up with to figure out their final form.

[aaclayton]

Originally in GitLab by @akrigline

I'll take a look at the UI flow and report back with some hook proposal updates.

[aaclayton]

Originally in GitLab by @akrigline

Since each step of the Advancement process (currently only LevelIncreasedStep) is an ancestor of AdvancementStep which itself is an ancestor of Application with popOut:false, these hooks are already handled for us.

The overall AdvancementPrompt fires renderAdvancementPrompt.
The individual steps fire renderLevelIncreasedStep (and renderAdvancementStep).

As such, this set of hooks is done.

[aaclayton]

Originally in GitLab by @akrigline

The resulting flow fires Advancement#levelChanged with parameters for item as well as information about the level increase (character and class item levels to be used for calculating the delta).

As a result, providing the following hook would allow for the stated use case, but also would allow for greater flexibility:

  levelChanged({ item, character, class: cls }) {
    // `this` is the `AdvancementPrompt`
    // `item` is the item causing the advancement flow to trigger
    // `character` and `class` are part of `LevelChangeData`
    // NOTE: I'm taking the `dnd5e.` namespace prefix from our existing polymorph flow's hook: dnd5e.transformActor
    const allowed = Hooks.call("dnd5e.preLevelChanged", this, item, character, cls);

    if (allowed === false) return;

    let levelDelta = character.final - character.initial;

[aaclayton]

Originally in GitLab by @akrigline

The final actor update would go through the normal document hooks so a specific one for Advancement is probably unnecessary.

This is untrue because of the Item CRUD step that also happens. So during AdvancementPrompt#commitUpdates, two hook calls would be valuable for module that wish to either modify or respond to the advancement process completing:

    const allowed = Hooks.call("dnd5e.preCommitAdvancementUpdates", updates, toCreate, toUpdate, toDelete);

    if (allowed === false) return Promise.resolve(this.actor);

    const actorWithUpdates = await Promise.all([
      this.actor.update(updates),
      this.actor.createEmbeddedDocuments("Item", toCreate, { skipAdvancement: true, keepId: true }),
      this.actor.updateEmbeddedDocuments("Item", toUpdate, { skipAdvancement: true }),
      this.actor.deleteEmbeddedDocuments("Item", toDelete, { skipAdvancement: true })
    ]);

    Hooks.callAll("dnd5e.commitAdvancementUpdates", actorWithUpdates);
    return actorWithUpdates;

[aaclayton]

Originally in GitLab by @akrigline

The "After a Step but before the synthetic actor is updated" hook is giving me some trouble. The goal is to enable modules to inject their own controls into a given Advancement Flow or Advancement Step, and then have those updates be picked up as the flow progresses.

It seems like during AdvancementStep#prepareUpdates is the safest place, and might look like so:

  prepareUpdates({ reverse=false }={}) {
    let preparedUpdates = this.flows.reduce((updates, flow) => {
      // ...
    }, { actor: {}, item: { add: {}, remove: [] } });

    // `preparedUpdates` expects to be mutated
    // `this` is the application instance, allowing access to both its `flows` and its html
    // the last arg will be the args of this function so the hook consumer knows if we're reversing or not
    Hooks.callAll("dnd5e.prepareAdvancementStepUpdates", preparedUpdates, this, { reverse });

    return preparedUpdates;
  }

This seems like it would allow a module to mutate the preparedUpdates before they're shipped off to the next step (either undoChanges or applyChanges), where they're committed to the synthetic actor.

@arbron does that look right? Can you think of a better place to put such a hook given the goal?

[aaclayton]

Originally in GitLab by @arbron

So placed here it will be dealing with all of the changes from the different flows merged together. I suppose the question is is it more useful to have it here, or inside the loop after the data from each flow's updates are prepared?

      // Prepare updates
      const flowUpdates = {
        actor: flow.advancement.propertyUpdates(fetchData),
        item: flow.advancement.itemUpdates(fetchData)
      };
      Hooks.callAll("dnd5e.prepareAdvancementFlowUpdates", flowUpdates, flow, { reverse });

      // Merge updates
      Object.assign(updates.actor, flowUpdates.actor);
      add.forEach(uuid => updates.item.add[uuid] = `${flow.advancement.parent.id}.${flow.advancement.id}`);
      updates.item.remove.push(...flowUpdates.item.remove);

[aaclayton]

Originally in GitLab by @akrigline

I'm not sure honestly. They feel like different use cases, so maybe both should be added in.

prepareAdvancementFlowUpdates to handle individual flow changes (use case might be like to change how the hitpoints flow works, adding INT instead of CON for instance)

This could even be dynamically named prepareAdvancement${flow.constructor.name}Updates or something, to let a module target one specifically

prepareAdvancementStepUpdates to handle larger overall changes to the step as a whole (use case would be like DIYing a new Flow outside of the existing flows, adding all of a caster's spells for that level at once for instance)

[aaclayton]

Originally in GitLab by @arbron

Yeah, then that could work. Keep in mind though that the system is designed to support totally custom advancement types without using hooks, so these should be focused more on modifying the behavior of the built-in step, rather than the individual advancement types themselves.

[aaclayton]

Originally in GitLab by @akrigline

Oh I didn't realize that. Definitely going to need a writeup from you about this before we release 😛

In that case, the one inside the loop is 'more proper' for sure. We can add the outside loop one later if someone comes up with a use case that isn't covered by implementing a custom flow.

[aaclayton]

Originally in GitLab by @akrigline

Exploring the AdvancmentManager after !512

The "Pre Advancement" Hook has gotten trickier as there's less of a centralized point where the workflow is kicked off.

It seems the best play now would be to inject a Hooks.call into AdvancementManager#render. This would allow a module to see and update any steps as needed, as well as completely abort the rendering of the AdvancementManager.

  render(...args) {
    const allowed = Hooks.call("dnd5e.preAdvancementManagerRender", this);
    if ( allowed === false ) {
      return;
    }

    if ( this.step?.automatic ) {
      if ( this._advancing ) return this;
      this._forward();
      return this;
    }

    return super.render(...args);
  }

[aaclayton]

Originally in GitLab by @akrigline

This is still mostly true, though the set of hooks is different slightly:

• renderAdvancementManager runs for every step
• render______Flow runs for every "flow" that is rendered, e.g. "renderItemGrantFlow"