Skip to content

Add button to show passive allocation order to passive tree tab (rebranch)#3546

Closed
Voronoff wants to merge 6 commits into
PathOfBuildingCommunity:devfrom
Voronoff:passive-allocation-order-rebranch
Closed

Add button to show passive allocation order to passive tree tab (rebranch)#3546
Voronoff wants to merge 6 commits into
PathOfBuildingCommunity:devfrom
Voronoff:passive-allocation-order-rebranch

Conversation

@Voronoff
Copy link
Copy Markdown
Contributor

@Voronoff Voronoff commented Oct 22, 2021

Rebranch of #3318 at @Wires77 request

Description of the feature added:

A button to show the order that passive nodes have been allocated on the tree for the build. This lets build creators have a single tree that people can follow the order of unless there are big respecs. Main tree and ascendency are tracked separately. Supports cluster jewel swapping, class switching, tree reset, and undo/redo.

Steps taken to verify a working solution:

Created testing builds with cluster jewels to swap around, tested old builds without existing allocation order for crashes, including swapping cluster jewels.

Screenshot:

https://imgur.com/aMppNoO

Copy link
Copy Markdown
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

Any reason you chose to not store the order alongside the nodes themselves? Seems like a cleaner solution to me...

When I have the champion node on Ascendant selected, and select the gladiator ascendancy, I get this crash:
image

The checkbox disappears in portrait mode, and when coming back to landscape mode, the text is overlapping:
image

Using shift to choose a path before selecting also results in some odd behavior:
image

At one point I was able to have the first part of my tree not have an allocation order and the second half started at '1'.

I'm not sure about the visibility of the red numbers next to nodes, honestly. I'm not exactly sure what would look better, maybe a full replacement of the node or a gradient from the beginning to the end. Might want to see what other people think.

Comment on lines +462 to +470
if a.allocationOrder == nil and b.allocationOrder == nil then
return false
elseif a.allocationOrder == nil then
return true
elseif b.allocationOrder == nil then
return false
else
return a.allocationOrder > b.allocationOrder
end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This logic can be simplified:

Suggested change
if a.allocationOrder == nil and b.allocationOrder == nil then
return false
elseif a.allocationOrder == nil then
return true
elseif b.allocationOrder == nil then
return false
else
return a.allocationOrder > b.allocationOrder
end
if b.allocationOrder == nil then
return false
elseif a.allocationOrder == nil then
return true
else
return a.allocationOrder > b.allocationOrder
end

local size = 175 * scale / self.zoom ^ 0.4
DrawImage(self.highlightRing, scrX - size, scrY - size, size * 2, size * 2)
end
if self.showAllocationOrder and node.allocationOrder ~= nil then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if self.showAllocationOrder and node.allocationOrder ~= nil then
if self.showAllocationOrder and node.allocationOrder then

Comment on lines +678 to +690
SetDrawLayer(nile, 29)
SetDrawColor(1, 0, 0)
local size = 100 * scale
local offset = node.size * 1.8 * scale
DrawString(m_floor(scrX - offset), m_floor(scrY - offset), "LEFT", size, "VAR", tostring(node.allocationOrder))
end
if self.showAllocationOrder and node.ascendancyAllocationOrder ~= nil then
SetDrawLayer(nile, 29)
SetDrawColor(1, 0, 0)
local size = 100 * scale
local offset = node.size * 1.8 * scale
DrawString(m_floor(scrX - offset), m_floor(scrY - offset), "LEFT", size, "VAR", tostring(node.ascendancyAllocationOrder))
end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can all be combined into two calls to a local function

Comment thread src/Classes/TreeTab.lua
end

self.controls.treeAllocationOrder.state = self.viewer.showAllocationOrder

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

end
end

ConPrintTable(reserverdAllocationOrders)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ConPrintTable(reserverdAllocationOrders)

for nodeId, unallocated in pairs(reserverdAllocationOrders) do
self:RemoveFromAllocationOrder(self.nodes[nodeId])
end
ConPrintTable(self.allocationOrder)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ConPrintTable(self.allocationOrder)

@LocalIdentity LocalIdentity added the enhancement New feature, calculation, or mod label Oct 31, 2021
@Wires77
Copy link
Copy Markdown
Member

Wires77 commented Feb 2, 2022

Decided against merging this even if the issues are fixed, as it's not enough added value for the amount of complexity it introduces. Build creators can still specify certain clusters in their notes as important or make different trees for different stages of a character

@Wires77 Wires77 closed this Feb 2, 2022
@Voronoff
Copy link
Copy Markdown
Contributor Author

Voronoff commented Feb 2, 2022

@Wires77 I never got a github notification about the corrections, whoops. I'm happy to fix it and I do think it adds a lot of value to automagically track the order, especially for POBs that don't get written up into fully complete guides, but if you don't want it that's okay too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature, calculation, or mod

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants