Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursive expansion of auto-opened exchangeables #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Will-Crain
Copy link
Contributor

This came about because, as a new player, I couldn't find the sources for some items. I had to manually go through each drop table to find, for example, where a elixirs come from. Or, on the other end, wondering why a lostearring1 shows crossbow in it's drop table, but a Crossbow doesn't list lostearring1 as a source

This PR fixes some weird edge cases where, for example, a Crossbow doesn't have a lostearring1 as a source.
Also fixes issues where elixirvit0 and similar elixirs don't show the exchangeable shell as a source.

Notably, this does not interfere with the comment from 2022 around this function, which removed showing exchangeables like lostearring1 from showing as a source for weaponbox, since lostearring1 auto-opens a weaponbox.

The solution is a new recursive internal function recur_replace. This scans the drop table of all exchangeable items and looks for an entry whose [1] index is 'open'. 'open' in this context means the drop table in the [2] index is **automatically opened**. If it finds one of these items, recur_replace replaces that drop table entry with the contents of the automatically opened drop table.

Then, in the loop where we find which exchangeable items to show as a source, we simply look through these modified, recursively expanded drop tables.

For example: the Crossbow, weaponbox, and lostearring1
lostearring1s drop table is simply

[[
	1,
	'open',
	'weaponbox',
]]

So, our function will see this and replace lostearring1s drop table with the contents of weaponbox. Then, when we iterate through these modified drop tables, we can find that lostearring1 could indeed drop a crossbow

Crossbow
weaponbox

EngineerYo added 2 commits January 21, 2024 21:54
Fixing some weird edge cases where, for example, a Crossbow doesn't have a `lostearring1` as a source
true_table is now a shallow copy of G.drops, so we don't mutate it when we call `recur_replace` on it
@Will-Crain
Copy link
Contributor Author

Will-Crain commented Jan 22, 2024

I found an issue in 4f3e37c where I was mutating G.drops when calling recur_replace on it. d71ecf3 now feeds recur_replace with a shallow copy of a drop table from G.drops, so no mutations on the original data occur

This error was hinted to me in a discord discussion I had about this fix, but I didn't realize the issue at the time. My code had mutated G.drops so I did not see the basicelixir drop table tucked inside G.drops.seashell

Functionality in "Obtainable From" list, as described in the original comment remains unchanged, but G.drops no longer mutates when creating this list :)

Copy link
Collaborator

@Telokis Telokis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thank you very much for your contribution! I have a slight edge case that I'd like to see handled but apart from that, it's great, thanks!

Below is the whole code I used to test this out:

const G = {
    drops: {
        weaponbox: [
            [1.4, "t2bow"],
            [1, "mace"],
        ],
        tempbox: [
            [1, "open", "weaponbox"],
            [1, "firestars"],
            [1, "bow"],
        ],
        tempbox2: [
            [1, "open", "weaponbox"],
            [1, "lunarmace"],
            [1, "sertgsth"],
            [1, "open", "tempbox"],
            [1, "mything"],
        ],
    },
};

/**
 * Replace all "open" entries in a drop table with the contents of the table it references.
 * @param {Array} table - The drop table to expand.
 * @returns {Array} - The expanded drop table.
 */
function expand_drop_table(table) {
    let changed = true;
    let newTable = [...table];

    while (changed) {
        changed = false;

        const tmpTable = [];
        for (const drop_item of newTable) {
            if (drop_item[1] === "open") {
                tmpTable.push(...G.drops[drop_item[2]]);
                changed = true;
            } else {
                tmpTable.push(drop_item);
            }
        }
        newTable = tmpTable;
    }

    return newTable;
}

console.log(expand_drop_table(G.drops.tempbox));
console.log(expand_drop_table(G.drops.tempbox2));

Comment on lines +1825 to +1831
function recur_replace(table) {
for (let [idx, drop_item] of table.entries()) {
if (drop_item[1] === 'open') table.splice(idx, 1, ...G.drops[drop_item[2]])
}

return table
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are modifying an array while iterating over it. There are edge cases where this will break things. (If the first element needs to be expanded, for example)

Below is a rewrite with some docs and a more descriptive function name:

/**
 * Replace all "open" entries in a drop table with the contents of the table it references.
 * @param {Array} table - The drop table to expand.
 * @returns {Array} - The expanded drop table.
 */
function expand_drop_table(table) {
    let changed = true;
    let newTable = [...table];

    while (changed) {
        changed = false;

        const tmpTable = [];
        for (const drop_item of newTable) {
            if (drop_item[1] === "open") {
                tmpTable.push(...G.drops[drop_item[2]]);
                changed = true;
            } else {
                tmpTable.push(drop_item);
            }
        }
        newTable = tmpTable;
    }

    return newTable;
}

Thanks for your ground work, it helped me greatly.

/*
[21/01/24] The below isn't sufficient to show that the Crossbow can be obtained from `lostearring1`. `lostearring1`'s droptable only includes an opened `weaponbox`
But because `lostearring1` doesn't directly & shallowly show that it drops a Crossbow, Crossbow's only source is `weaponbox`
This can lead to confusion. Instead, a recursive expansion of auto-opened drop tables is peformed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the previous code instead of commenting it. But we can keep the comment explaining things.


// shallow copy of G.drops[temp_name], otherwise we mutate it
var table = [...G.drops[temp_name]]
var true_table = recur_replace(table)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a more descriptive name for the expanding function.

@Telokis
Copy link
Collaborator

Telokis commented Apr 12, 2024

Hey @EngineerYo! Thanks a lot for this PR!
Let me know if you don't have the time to implement my reviews, I'll do them myself, no worries!

@Will-Crain
Copy link
Contributor Author

@Telokis -

Thanks for these comments! They're both very functional, and also educational for me 😄

I'm out of town with only a work laptop + my phone until April 22nd. If you'd like to implement these changes before then, by all means.

Otherwise, I'll revise the week I'm back home

@Telokis
Copy link
Collaborator

Telokis commented Apr 12, 2024

Thanks for these comments! They're both very functional, and also educational for me 😄

I'm out of town with only a work laptop + my phone until April 22nd. If you'd like to implement these changes before then, by all means.

Otherwise, I'll revise the week I'm back home

@EngineerYo Hey, thanks for answering! If you're motivated to fix the PR I'll let you do it, thanks!

@Telokis Telokis added Type: enhancement Changes the existing content of the game Status: waiting for author The maintainers are waiting for the author or contributor to reply or perform changes Scope: UI/UX This is related to the user interface or user experience labels Aug 13, 2024
@Telokis
Copy link
Collaborator

Telokis commented Aug 13, 2024

@Will-Crain Hey! How do you feel about this PR? Do you still wanna work on it or should I close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: UI/UX This is related to the user interface or user experience Status: waiting for author The maintainers are waiting for the author or contributor to reply or perform changes Type: enhancement Changes the existing content of the game
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants