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

Added tree comparison #2151

Merged
merged 4 commits into from
Feb 13, 2021
Merged

Added tree comparison #2151

merged 4 commits into from
Feb 13, 2021

Conversation

Ethrel
Copy link
Contributor

@Ethrel Ethrel commented Feb 9, 2021

Adding tree comparison

Relates to #2141

Realized it was just LUA, so it wasn't a huge deal to just throw my hat in the ring for a minute.

@Quotae
Copy link
Contributor

Quotae commented Feb 9, 2021

This is a great first PR, very useful feature. Would like to see a few changes though:

  1. Would be nice if this extra tree dropdown disappeared when you're not comparing them, instead of being grayed out. The node power buttons disappear when not in use, check out how they do it.
    image

  2. Spacing between the Compare: text and the button to the left of it is a bit off versus others. See:
    Yours: image
    Others: image

  3. Issue: if you are comparing trees and also showing node power, the colors of the tree comparison don't work correctly:
    image

  4. Trying to compare trees of two different game versions causes a crash. Here's a build with a 3.12 tree in case you don't have one: https://pastebin.com/GBYMZDDV Just open the build, head to the tree tab, click "Convert to 3.13" at the bottom, and try to compare one of the 3.12 trees to the 3.13 one.

@Ethrel
Copy link
Contributor Author

Ethrel commented Feb 9, 2021

Ok. So, I updated the commit, squashed it all down, we're now all nice and happy in a single commit.

Hiding the dropdown was a little more complicated than the node power version. The node power version has the advantage of being able to just hide everything anchored to it. In my case, that couldn't be done like that, so I had to do some things (See code block at 221ca7f#diff-5fcc1cbe087d23273aa0779a6e056d56b04c07acef5ce3f943b9cd402048353fR74). Hopefully that's how it's meant to be done rather than just some hack.

@ppoelzl
Copy link
Member

ppoelzl commented Feb 9, 2021

Please no unrelated changes to the formatting. This makes it harder to review than necesary.

@Ethrel Ethrel closed this Feb 9, 2021
@Ethrel Ethrel reopened this Feb 9, 2021
@Wires77
Copy link
Member

Wires77 commented Feb 10, 2021

I get this error in the Items tab when hovering over a socketed jewel. Using the pastebin that @Quotae posted, even with the tree comparison off.

image

@Ethrel
Copy link
Contributor Author

Ethrel commented Feb 10, 2021

Didn't even realize that the jewel's tree position was drawn on jewel hover. Just a method call that didn't match the new signature. Fixed that now too :D

@Wires77
Copy link
Member

Wires77 commented Feb 10, 2021

Didn't even realize that the jewel's tree position was drawn on jewel hover. Just a method call that didn't match the new signature. Fixed that now too :D

Oh! Then I'm guessing...yep, this error appears in the Calcs tab, too. Basically on any stat that has Tree as the Source you can hover over the Source Name and see an inset tree view.

image

I'd run a file search for the method you changed, in case there are other places I can't think of right now

@Ethrel
Copy link
Contributor Author

Ethrel commented Feb 10, 2021

Okay. So when you pointed out another place, I did indeed do a full source search. Found the one for the calcs tab with my search, and that seems to be the only one left. And that one method was the only one that wasn't a function defined within a local scope of somewhere, so it should be the only breakpoint for the same incorrect method arguments call issue.

Copy link
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.

The functionality works great, I finally got a chance to look at the code itself and made some suggestions. Once those are addressed this looks good to go!

Classes/TreeTab.lua Outdated Show resolved Hide resolved
Classes/PassiveTreeView.lua Outdated Show resolved Hide resolved
Classes/PassiveTreeView.lua Outdated Show resolved Hide resolved
@Ethrel Ethrel requested a review from Wires77 February 12, 2021 17:23
@Wires77 Wires77 added the enhancement New feature, calculation, or mod label Feb 13, 2021
Copy link
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.

LGTM

@Wires77 Wires77 merged commit efcebe2 into PathOfBuildingCommunity:dev Feb 13, 2021
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.

4 participants