zarillion / handynotes-plugins

A collection of HandyNotes plugins for World of Warcraft.
Other
48 stars 34 forks source link

Add group to TomTom broken #272

Closed RedHawker closed 1 year ago

RedHawker commented 1 year ago

ADDON VERSION: Dragonflight v30

Describe the bug "Add group to TomTom" feature no longer works, it only adds the first.

adavak commented 1 year ago

Unable to reproduce this error, please elaborate on which node is causing the problem.

RedHawker commented 1 year ago

The Disgruntled Hunter achievement Disturbed Dirt locations Elemental Storms etc.

adavak commented 1 year ago

20230126195533

no error by my tested.

RedHawker commented 1 year ago

I don't know what that large dialog is showing?

RedHawker commented 1 year ago

The multi group support changes seems to have broken this, since instead of being a single entry, the group field is now a table. So in https://github.com/zarillion/handynotes-plugins/blob/master/core/tomtom.lua#L21 where it compares the peerNode.group to node.group, it's comparing the wrong things.

If I change it to compare peerNode.group[1] to node.group[1] it works as it previously did, but this obviously breaks down if a node is in multiple groups.

For the "Add group to TomTom" to continue working, you'll currently have to check peerNode's groups for every group in node instead of the simple peerNode.group == node.group that it currently is.

RedHawker commented 1 year ago

Not saying this is great, but this is what I mean:

local function AddGroupWaypoints(node, mapID, coord)
    local map = ns.maps[mapID]
    for peerCoord, peerNode in pairs(map.nodes) do
        if peerNode:IsEnabled() then
            local added = false
            for _, gv in pairs(node.group) do
                for _, pgv in pairs(peerNode.group) do
                    if gv == pgv then
                        AddSingleWaypoint(peerNode, mapID, peerCoord)
                        added = true
                        break
                    end
                end
                if added then
                    break
                end
            end
        end
    end
end
Ioney commented 1 year ago

I totaly forgot about the tomtom feature when adding the multigroup support. I added the peerNode.group[1] to node.group[1] solution as an easy fix, which is the most fitting for me because the first group should always be the main group, for example Rares, the other groups are kinda just subgroups if the rare is also part of an achievement.

The multigroup (or subgroup if you want) feature is not used right now, however i can see some cases where it could be usefull. We may also revert back to just a single group per node in the future if it turns out to be not as usefull as i thought.

Krovikan commented 1 year ago

v31 is out! :)

RedHawker commented 1 year ago

Confirming that the fix works.

Thank you!