Jagusti/fvtt-wfrp4e-gmtoolkit

Advantage not being handled properly for player characters

Clapolad opened this issue · 5 comments

Clapolad commented

Checklist

  • I have disabled GM Toolkit and the issue doesn't happen.
  • I have disabled all other modules (except GM Toolkit) and the issue still happens.
  • I go not get a Spectator Notification related to actors affected by this issue.
  • I can be reached on Discord by my handle: clapo

Versions

  • Foundry VTT: v11.311
  • WFRP4e System: v7.0.2
  • GM Toolkit: v6.0.5
  • Macro (if applicable): Add Advantage

Description

What should happen

Adding or removing advantage using the macro or automated system would work on player characters.

What is happening

Adding and removing advantage using the macro or automated system only works for npcs. (Edit: this only happens if the gm wants to edit the advantage, the macro works fine for players, the automation is still broken)

Steps to reproduce the issue

  1. Select / Target Player character
  2. Execute macro Add/Remove advantage or enable automated advantage
  3. Notification comes up but with no update to the advantage number

Images

image

Jagusti commented

Thanks for raising the issue. Think I'm going to need more info.

  • Can you share any console log output from when the process please, whether successful or errors. A screencast would be handy.
  • Does this happen with just Isaiah or other player characters?
  • Can you export and share a character it does and one that it doesn't work for?
  • The character is in an active combat right?
  • Does the same happen whether you have Group Advantage enabled or disabled?
  • Do you have these options all enabled?

image

Thanks

Clapolad commented
Jagusti commented
RavenknightDM commented

I had the same issue with player characters after installing Up-in-Arms and oddly the issue went away after I duplicated the characters and used the duplicates instead of the originals. Why? I have no idea. :D

Forien commented

Issue is caused by improper checking of ownership and several wrong assumptions (that there is only one GM and all Documents were manualy and directly created by said GM)

Let me explain.

In Foundry, ownership is an object, which in it's most concise form can simply be {default: 3}. 3 represents a level of Ownership as can be checked in CONST.DOCUMENT_OWNERSHIP_LEVELS.

Then, any deviation from default must be applies to specific user by user id. For example: {default: 3, "QkizEELLge2OGlxk": 0}.

When user creates a Document, Foundry gives the Document the following ownership:

{
  "default": 0,
  "QkizEELLge2OGlxk": 3
}

What about GM?

GM is considered Owner of every and any Document in the World, no matter the default or explicit level.

So what was the issue?

When Player creates a Document (like an Actor), or an Actor is created programatically, there is no guarantee it will show GM's ID in there, which shows only as a side effect of how Foundry generates default ownership, and only if the specific GM creates that Document.

The issue would also appear in Worlds with multiple GMs, if one was that created the Document, then the other would not explicitly appear in the Ownership object.

And this Module simply checks for ownership[game.user.id], which has multiple issues:

  1. It may and often will return false for GMs if they are not the ones who created the Document
  2. It will return true for Limited and Observer levels, who have no permissions to edit Actors

Instead it is better to use Foundry's built in getter ClientDocument.isOwner (or if we want to test specific ownership level ClientDocument.testUserPermission), which code is as follows:

    get isOwner() {
      return this.testUserPermission(game.user, "OWNER");
    }

    testUserPermission(user, permission, {exact=false}={}) {
      const perms = DOCUMENT_OWNERSHIP_LEVELS;
      const level = user.isGM ? perms.OWNER : this.getUserLevel(user);
      const target = (typeof permission === "string") ? (perms[permission] ?? perms.OWNER) : permission;
      return exact ? level === target : level >= target;
    }

How this relates to the problems with Advantage?

When player characters attack, it's the GM who rolls for the defense in majority of times, so all scripts based on finished Opposed Tests run on GM side. Including the code that should award Player Characters the Advantage.
However, module wrongly thinks that GM has no ownership over Player Characters, so it's not applying the changes.

Steps to Reproduce

  1. Give Players Create Actors permission
  2. Log in as Player and create an Actor
  3. Being logged in as both Player and as GM, initiate a Combat
  4. Attack as a player
  5. Defend as a GM
  6. Observe no advantage being added to player

I made Pull Request to adress the issue:

  • ❤️1