Silverfeelin / SkyGame-Planner

Sky:CotL unlock planner/tracker
Other
17 stars 3 forks source link

Implementation of #48 unlock connected nodes #96

Closed PlutoyDev closed 2 months ago

PlutoyDev commented 2 months ago

Unlock prerequisite nodes if it isn't unlocked #48. Also implemented the opposite action of locking dependent nodes if prerequisite is lock.

Both actions require user confirmation.

Silverfeelin commented 2 months ago

Hey Plutoy. Thank you for your contribution! I will have a look at this when I get home from work in a few hours.

Silverfeelin commented 2 months ago

I've given this some thought a long time ago and opted not going for it just yet. I think it's a good feature to have but I'd prefer to add a setting for this. That way you don't get have to confirm it every time but still have the freedom to choose which method to use.

There were some changes in another branch that I think would be ideal for storing this preference. I've brought those changes over in #98 (specifically _storageService.getKey() and setKey()).
I'll open a PR to your branch that adds a setting which can be used to unlock the connected nodes.

There's a few things that still need to be addressed in these changes.

Helper functions

NodeHelper.trace(node) can be used to collect all nodes preceding the given node (including the node itself). With this function you can simply loop over the array this function returns to check what preceding nodes need to be unlocked.

NodeHelper.all(node) collects all nodes beyond the given node (including the node itself). With this function you can loop over the array to check what nodes need to be locked.

Nodes vs items

In my data every spirit tree has its own nodes and these nodes can reference the same item. This way I have all the historic trees available and can track which specific node was unlocked. For example with Sassy Drifter that saw a price change in 2021 and a tree change in 2024. In this screenshot you can see how I've tracked my missing hat in 2021 that I've unlocked in 2022.

image

Right now with your code it will unlock a node even when the item is already unlocked through another version of the spirit tree. This means you can now own two different nodes of the same item, which will both end up green. In the above example the wing buff and level 1 emote would end up green in both trees when I unlock the hat in the 2022 tree.

The difference between nodes and items is determined by node.unlocked and node.item.unlocked.

So the combinations that exist are:

PS. When a node with an orange checkmark is clicked it will lock that node. In this edge case I think it's best to stick to only locking this node so we don't have to change the other tree (that may not even be visible on the page).

In this Days of Fortune example (2024 left, 2023 right), I'd expect locking the warp to only lock the two wing buffs spells and bull mask, not the tiger mask.

image

I think the code to accomplish this (locking) wouldn't have to be much more than:

// Find all subsequent nodes that are unlocked *in this tree*.
const nodes = NodeHelper.all(node).filter(n => n.unlocked);
for (const node of nodes) {
    /* lock node */
}

And for unlocking:

// Find all preceding nodes where the *item* is not unlocked yet.
const nodes = NodeHelper.trace(node).filter(n => !n.item.unlocked);
for (const node of nodes) {
    /* unlock node */
}
Silverfeelin commented 2 months ago

PS. these helper classes with no dependencies (unlike services such as NodeService) exist so they can be accessed wherever, including the console. I use this occasionally to look up some data quickly like this random example of hats from season spirits:

var trees = window.skyData.spiritConfig.items.filter(s => s.type === 'Season' || s.type === 'Guide').map(s => s.tree);
trees.flatMap(t => NodeHelper.all(t.node).map(n => n.item)).filter(i => i.type === 'Hat').map(i => i.name);

And yes these functions were very much just written as I needed them.

PlutoyDev commented 2 months ago

I have made the recommended changes. Please review. Thank you

Silverfeelin commented 2 months ago

I've made some small changes to handle these scenario's:

In this Days of Fortune example (2024 left, 2023 right), I'd expect locking the warp to only lock the two spells and bull mask, not the tiger mask.

When a node with an orange checkmark is clicked it will lock that node. In this edge case I think it's best to stick to only locking this node so we don't have to change the other tree (that may not even be visible on the page).

Other than that the changes seem great so I will go ahead and merge this PR. Thank you!

Silverfeelin commented 2 months ago

I've added your name to the info page in https://github.com/Silverfeelin/SkyGame-Planner/commit/4a861a43e45038e201eef731fee2aa1b8282914b. Let me know if I should change it to PlutoyDev.