ultrakorne / better-rolltables

Module for FoundryVTT to improve rolltables
MIT License
32 stars 29 forks source link

Rolling from Macro displays "undefined" in Chat Message #112

Closed Shuggaloaf closed 2 years ago

Shuggaloaf commented 3 years ago

When rolling from a macro, for example by using game.tables.get("TABLE_ID").draw();, if there is not a description in the table, it displays "undefined". Like this: image

Adding a space to the description temporarily fixes the issue, but the space is removed after a refresh and the problem returns. Adding a whitespace character or similar can be used as a work-around, however it adds an extra blank line to the roll, which is undesirable. As you can see: image

MiahNelah commented 3 years ago

Thanks for your feedback! Better-rolltables is not triggered when using vanilla draw method. Table are rolled as default on your screenshots. You should use the following code in your macro:

const table = game.tables.get("TABLE_ID");
game.betterTables.betterTableRoll(table);

Please see wiki for more information: https://github.com/ultrakorne/better-rolltables/wiki/API-for-macros-and-modules#how-to-roll-tables-from-macros

Let me know if this fix your issue.

Shuggaloaf commented 3 years ago

Sorry about that @Daimakaicho, I didn't even catch that. I actually have been to the Wiki and did end up changing the macro for other reasons and am using the code you suggested. Didn't put 2 and 2 together or would have came back here and closed this. Apologies!

While I have you, and since I did check the wiki and I didn't see anything like this there, is there a way to accomplish something like this with Better Rolltables? Relevant code: let table = game.tables.getName("TableName") let draws = await(table.roll()); var message = 'This is result 1: ${draws.results[0].data.text}, Result 2: ${draws.results[1].data.text}'

Tried a few different things and couldn't get a good result. Appreciate it!

MiahNelah commented 3 years ago

Do you want something like this?

const table = game.tables.getName("TableName");
const roll = await table.roll({async: true});
const message = `This is result 1: ${roll.results[0].data.text}`;
await ChatMessage.create({
    content: message
});
Shuggaloaf commented 3 years ago

Thanks for the response. Thanks for the new info on changing let to const an adding {async:true}. I'm still getting the same result as before however.

The problem is that it still displays results as plain text when this is a Loot Table. I was hoping there was a way to replace table.roll with something like table.generateChatLoot but haven't found a way to do so.

Is this just not possible with a loot table?

MiahNelah commented 3 years ago

If I understand your need (but not sure), the first snippet should give you the good output:

const table = game.tables.getName("TableName");
game.betterTables.betterTableRoll(table);

image

Shuggaloaf commented 3 years ago

Sorry for not being more clear on what I'm trying to do. I'm trying to have the results of the rolls show inside a block text in a chat message. I can do this fine with a standard table but haven't found out how to do it with BRT.

So to give an example that hopefully helps, I currently have a macro to do this with "rare coins". This is just a standard text (i.e. non-Loot) example and the code looks like this:

let table = game.tables.getName("TblRareCoins")
let draws = await(table.roll());
msgHead = `<p style="font-size:22px; background:purple; color:white";><b> You have found Treasure!</b></p>`

var message = msgHead + `[[d20]] ${draws.results[0].data.text}, ${draws.results[1].data.text} ${draws.results[2].data.text}. <br><br> One side is marked with ${draws.results[3].data.text}. You would judge the age as ${draws.results[4].data.text}. <br><br> It's value is defined as '${draws.results[5].data.text}'. The coins ${draws.results[6].data.text}` 

ChatMessage.create({
    content: message,    
});

The output from that macro looks like this:

What I'd love to figure out is how to do this with loot items so that I could incorporate them into a block of text as well. So, for a simple example, I might have something like "You dig through their pockets and find a [horseshoe] and a [moss agate]. In their pack, you find a book titled [The 13 Habits of Highly Effective Necromancers]." (where items in brackets would be actual loot).

Shuggaloaf commented 3 years ago

I also admit I may be going about this the wrong way and that the ${draws.results[0].data.text} and similar may not be needed and there may be another way to do this. (I'm pretty new to JS). So I'm open to any method that gets the same end result.

MiahNelah commented 3 years ago

If I understand it well, you want to recreate result link like BRT does in classic results ?

Shuggaloaf commented 3 years ago

Not 100% sure what you mean by that unfortunately. I am about to jump into a meeting but I will try to mock something up in photoshop to give you a better picture of what the result would look like.

MiahNelah commented 3 years ago

Do you want to do something like this? image

Shuggaloaf commented 3 years ago

Ah man I should have checked back here before making the image below! lol Well I made it so I'm posting it!! :-)

So yeah, that's pretty much it exactly.

MiahNelah commented 3 years ago

The good news is all interactivity is handle by foundry itself. The bad news is you have to write some special HTML to allow foundry to handle everything. Hopefully it's not that hard and you should have every needed info from table roll result. Here is a sample:

// Get rolltable and roll it
const table = game.tables.getName("TableName");
const roll = await table.roll({async: true});

// For each roll result, we create an HTML element with every needed info to create a clickable object
// This is not perfect, some cases are not handled (icon is wrong, text only results, etc)
const links = roll.results.map(result => {
    const id   = result.data.resultId;     // the item ID
    const pack = result.data.collection;   // the item collection (if any)
    const name = result.data.text;         // the item name

    const collection = game.collections.get(result.data.collection) || game.packs.get(result.data.collection);
    if (collection) {
        const item = collection.get(result.data.resultId);
        const documentName = item.documentName;

        return `<a class="entity-link" draggable="true" data-id="${id}" data-entity="${documentName}" data-pack="${pack}"><i class="fas fa-suitcase"></i>${name}</a>`;
    }
}).filter(x=>x);

// We create message
const message = `You enter the room. A ${links[0]} is on the floor. Do you take it?`;
ChatMessage.create({
    content: message
});

I can't test it now, there is maybe some tweaks to do.

Shuggaloaf commented 3 years ago

This is definitely getting a bit outside my comfort zone, but I'm willing to give it a go. After testing, it doesn't seem like it likes this line: const count = item.flags["better-rolltables"]["brt-result-formula"].formula; and give me the following console error Uncaught (in promise) TypeError: Cannot read property 'brt-result-formula' of undefined.

(No rush on this by the way. I know you're not able to test right now)

MiahNelah commented 3 years ago

Remove this line for now.

Shuggaloaf commented 3 years ago

did so and the message then displays and looks correct, but (1) the item is unable to be dragged into inventory and (2) I get this console error:

Uncaught TypeError: Cannot read property 'entity' of undefined
    at HTMLAnchorElement._onDragEntityLink (foundry.js:21413)
    at HTMLBodyElement.dispatch (jquery.min.js:2)
    at HTMLBodyElement.v.handle (jquery.min.js:2)
Shuggaloaf commented 3 years ago

Sorry, to clarify, I get that error when trying to drag to inventory

MiahNelah commented 3 years ago

So I guess output is good for you. This is drag behavior is quite weird however. I need to double check what I wrote. Do your items in rolltable come from compendium or your campaign (Items tab)?

Shuggaloaf commented 3 years ago

Your edit helped me figure it out. The table I was using as a test didn't have legit items in it. Once I changed to a table based on 5e adventure gear it works perfectly!

Is there any consequences of removing that item.flags line?

I really appreciate you taking the time to walk through this with me and help get it working.

MiahNelah commented 3 years ago

Line with item.flags allow you to get the count drawn from table (for instance, you may have 4 healing potion from a table draw). So if you don't need this count, this line is useless. I write it as sample, but never used it. Edit I made will work until your work with Item objects. This will fail with Actor one for instance. I need to write a more robust snippet which check object type.

MiahNelah commented 3 years ago

I updated my snippet with a more complete version which should work with every type of object, inside or outside of compendiums.

Shuggaloaf commented 3 years ago

Hmmm... now I get

Uncaught (in promise) TypeError: Cannot read property 'documentName' of undefined
    at eval (eval at _executeScript (foundry.js:14665), <anonymous>:18:35)
    at Array.map (<anonymous>)
    at eval (eval at _executeScript (foundry.js:14665), <anonymous>:10:28)

edit also just noticed the pre-edit code doesn't work with multiple draws. So for example const message = 'You enter the room. ${links[0]}, ${links[1]} and ${links[2]} are on the floor. Do you take them?';

edit2: I was able to get multiples working by doing this:

const roll = await table.roll({async: true});
const roll2 = await table.roll({async: true});
const roll3 = await table.roll({async: true});

and also just copy/pasting that whole const links chunk of code and doing links2, links 3, etc.

But obviously this is sloppy and I'm guessing there's a better way to do it. Just don't have the knowledge to get there.

MiahNelah commented 3 years ago

I'm thinking about writing an helper method to generate link, so you'll just have to call it. About multiple calls, are you sure you set your table to be rolled multiple times? You should have as many results as you set in your tables. I'll take some time and do deeper tests.

Shuggaloaf commented 3 years ago

I only had the table set to 1 roll, but even after updating it to 3, and clicking update, it still shows "undefined" for rolls 2 and 3. (This is with your 1st version of the macro).

so here is what I am using for the macro:

const table = game.tables.getName("Gear Loot");
const roll = await table.roll({async: true});

const links = roll.results.map(result => {
    const item = result.data;
    const id = item.resultId;                                                     // the item ID
    const pack = item.collection;                                                 // the item collection (if any)
    const name = item.text;                                                       // the item name
    return `<a class="entity-link" draggable="true" data-id="${id}" data-pack="${pack}"><i class="fas fa-suitcase"></i>${name}</a>`;
});

const message = 'You enter the room. ${links[0]}, ${links[1]} and ${links[2]} are on the floor. Do you take them?';
ChatMessage.create({
    content: message
});

and here is the table with a few entries for you to see:

and this is the generated chat message with the "undefined" results highlighted:

Hope this helps! Thanks for working with me and it's not at all high priority so no worries on it taking time to research. This could even just be set to some future feature addition. I appreciate your efforts!

Shuggaloaf commented 3 years ago

Thanks for your feedback! Better-rolltables is not triggered when using vanilla draw method. Table are rolled as default on your screenshots. You should use the following code in your macro:

const table = game.tables.get("TABLE_ID");
game.betterTables.betterTableRoll(table);

Sorry to jump back to the original issue but there IS something going on with that. As you can see in the GIF below this is setup correctly AND it was working correctly but suddenly it now isn't. (It also does happen when a non-blank result is returned as well, like in the screen shots in original post)

Capture-02

MiahNelah commented 3 years ago

I only had the table set to 1 roll, but even after updating it to 3, and clicking update, it still shows "undefined" for rolls 2 and 3. (This is with your 1st version of the macro).

so here is what I am using for the macro:

const table = game.tables.getName("Gear Loot");
const roll = await table.roll({async: true});

const links = roll.results.map(result => {
    const item = result.data;
    const id = item.resultId;                                                     // the item ID
    const pack = item.collection;                                                 // the item collection (if any)
    const name = item.text;                                                       // the item name
    return `<a class="entity-link" draggable="true" data-id="${id}" data-pack="${pack}"><i class="fas fa-suitcase"></i>${name}</a>`;
});

const message = 'You enter the room. ${links[0]}, ${links[1]} and ${links[2]} are on the floor. Do you take them?';
ChatMessage.create({
    content: message
});

and here is the table with a few entries for you to see:

and this is the generated chat message with the "undefined" results highlighted:

Hope this helps! Thanks for working with me and it's not at all high priority so no worries on it taking time to research. This could even just be set to some future feature addition. I appreciate your efforts!

Thanks, I'll be able to reproduce with this.

MiahNelah commented 3 years ago

Ok I understand and there is some kind of evolution to do to support what you want to do with better-rolltables.

Vanilla roll method is handy because it returns results without posting message instantly. So you can reuse results to do what you want. Better-rolltable's roll method do not behave like this, and post message without returning results. It looks like a rewrite is needed to get a vanilla-like behavior. I'm afraid it won't be easy as BRT design is more a alternative to vanilla than an enhancement in background.

So, for your use case, you'll have to use vanilla roll method and call it as many time as you want. Rolltable will act as default table, and enhancement will be ignored.

EDIT: this script should be better suit for your needs:

async function createLink(table) {
    const result = (await table.roll({async: true})).results[0];
    const item = result.data;
    const id = item.resultId;               // the item ID
    const pack = item.collection;           // the item collection (if any)
    const name = item.text;                 // the item name
    return `<a class="entity-link" draggable="true" data-id="${id}" data-pack="${pack}"><i class="fas fa-suitcase"></i>${name}</a>`;
}

const table = game.tables.getName("Gear Loot");
const link1 = await createLink(table);
const link2 = await createLink(table);
const link3 = await createLink(table);

const message = `You enter the room. ${link1}, ${link2} and ${link3} are on the floor. Do you take them?`;
ChatMessage.create({
    content: message
});
Shuggaloaf commented 3 years ago

EDIT: this script should be better suit for your needs

Nice! I didn't even have to change the number of rolls on the table (it is back to 1) and from what I see here, if I want more results I just need to add a new const link# = line. Awesome!

You just couldn't walk away from it until you figured it out huh? ;-) hahaha I really appreciate it. Again thanks for all your help. I love pushing the limits of what Foundry can do to come up with cool ways to do things. Always appreciate devs who like working with users.

Just making sure, did you see my other post about the actual original issue of this thread? I put in a GIF to show you that even with using the correct BRT syntax, it is still showing "undefined". (See post with GIF above).

MiahNelah commented 3 years ago

Thanks for you rnice words :) Yes I saw your feedback, I'll look into next week I think, I notice I had a lot of work to do again to fix misbehavior.

Shuggaloaf commented 3 years ago

I'll look into next week I think,

No problem at all and wasn't trying to be pushy, just wanted to make sure it wasn't lost in that wall of text up there. :-)

MiahNelah commented 3 years ago

So, I dig a little into this issue... Honestly, it's a mess 😞 This module is not design at all to allow a more fine-grained control over results. I'm thinking about a solution for this, but for for now I can't think to something that won't fit well into this module...

Shuggaloaf commented 3 years ago

Sorry you're having to deal with a mess. :-/

Not sure if it helps but this problem did not exist a few months ago. I only noticed it in one of the more recent updates, so it must have been something that was changed that caused this.

My guess is that this started sometime on or after version 1.6.5. If you tested pre-1.6.5 in a 0.7.X instance of Foundry you will not see this problem. Maybe that will help you track down what change caused this issue?