hythloday / VenturePlanSoDMissions

Addon to bring VenturePlan up to date with the 9.1 missions
GNU General Public License v3.0
134 stars 36 forks source link

Companion list UI reverts to original with > 20 companions #33

Closed woefulwabbit closed 3 years ago

woefulwabbit commented 3 years ago

With Renown 62 and new companions from Torghast, it is possible to have 21 companions, and this causes left panel to revert the companion list from a Grid layout to the original layout when selecting companions for a mission.

Not sure if this is something that can be fixed through the global or has to be done in the original AddOn

solkarnar commented 3 years ago

The original venture plan addon also started to spawn lua errors when i recruited my 21st companion

ctompos commented 3 years ago

There's a simple fix for this, although I'm sure someone more comfortable with the code base and LUA can come up with something more elegant.

Open Widgets.lua in a text editor and go to line 2066.

Change the loop to iterate through a higher number, for example, 30:

for i=1,30 do

Save and reload your UI. Enjoy!

Cr42yC4r1 commented 3 years ago

inside function Factory.FollowerList for me it was line 1960

solkarnar commented 3 years ago

There's a simple fix for this, although I'm sure someone more comfortable with the code base and LUA can come up with something more elegant.

Open Widgets.lua in a text editor and go to line 2066.

Change the loop to iterate through a higher number, for example, 30:

for i=1,30 do

Save and reload your UI. Enjoy!

Updated my addon and looks ok now. Thanks!

kerashistorm commented 3 years ago

Line 1961 for me

The line you're looking for will say

for i=1,20 do

Change the 20 to something higher and you're good to go

N6REJ commented 3 years ago

I can confirm this fixes things. image

Gaviin1242 commented 3 years ago

Thanks for the fix, guys!

One thing to note though, the grid view doesn't appear to have been designed for this many companions. The top row is now pushed up out of the window so you can't see the "Troops" header. Should be ok for now, but if 4+ more companions are added and thus create another row, it may start to cause trouble.

Best option would probably be to add a scroll bar to the companion grid, or perhaps change the width/height of the overall window. Both of those options are probably pretty involved though from a coding standpoint.

Ignie commented 3 years ago

I only increased it to 24 and it worked fine for me. Past renown 62, there's only one more follower to be added at renown 71. The only other follower I've seen with recent speculation is Ben Howell who was datamined. No one on Wowhead has said they've found him but, even if they had, that would leave us with 23 followers.

BloodyFess commented 3 years ago

I only increased it to 24 and it worked fine for me. Past renown 62, there's only one more follower to be added at renown 71. The only other follower I've seen with recent speculation is Ben Howell who was datamined. No one on Wowhead has said they've found him but, even if they had, that would leave us with 23 followers.

Trying increase to 24 only - it doesn`t matter. The top row is pushed up out of the window anyway :(

N6REJ commented 3 years ago

I'm not seeing that issue.. image

kerashistorm commented 3 years ago

I'm not seeing that issue.. image

It happens when you have 21 companions. You have 19, so it won't affect you yet.

N6REJ commented 3 years ago

agreed.. let me see if I can find a quick fix. image

zealvurte commented 3 years ago

You would need to make quite a few edits to make it a scroll frame instead.

I've also opted to moved troops to the bottom, so that (at least for 9.1) I can see the troop header + all companions, as you rarely need to place troops with the Cursed Tactical Guide anyway.

LukasGrove commented 3 years ago

I was playing with this a fair bit, you can make all the icons smaller by multiplying all the sizes in function Factory.FollowerListButton(parent, isTroop) by .9 (or whatever size you want) down to the t:SetSize(46, 46). This will make them them all smaller, then you can adjust the row height by changing the 72 here: for i=1,30 do t = CreateObject("FollowerListButton", f, false) t:SetPoint("TOPLEFT", ((i-1)%4)76+14, -math.floor((i-1)/4)*72-130) s.companions[i] = t But the frame is still messed up, and after adjusting it it only worked on my character with 21 followers (the frame wasn't right for my alts with less).

Not sure what part is dynamically creating the frame larger, but it's pretty easy to shrink the portraits a bit and move the rows closer together if someone can find how to fix the dynamic frame size.

LukasGrove commented 3 years ago

I guess you can close it, just making the change listed in #35 doesn't do anything about the frame being messed up and the labels being pushed off the window (which is what we were discussing). It'll be ok until we get >24 followers, at which point it won't be possible to select troops anymore without fixing the frame.

epiktetov commented 3 years ago

Just in case if anyone's interested, a quick fix for 21st companion, moving the first companion frame to the empty space above the companion list:

  for i=1,25 do
    t = CreateObject("FollowerListButton", f, false)
--  t:SetPoint("TOPLEFT", ((i-1)%4)*76+14, -math.floor((i-1)/4)*72-130)
    t:SetPoint("TOPLEFT", ((i-2)%4)*76+14, -math.floor((i-2)/4)*72-130)
    s.companions[i] = t
  end

That looks pretty good (to my eyes) and will work for 22nd too, if using '(i-3)' instead, but of course not for 23rd (there's no more empty space in the frame)

TruthNZ commented 3 years ago

For a more flexible version, which will adjust to however many followers they add in:

for i=1,#C_Garrison.GetFollowers(123) do
    t = CreateObject("FollowerListButton", f, false)
    t:SetPoint("TOPLEFT", ((i-1)%4)*76+14, -math.floor((i-1)/4)*72-130)
    s.companions[i] = t
end
zealvurte commented 3 years ago

Using #C_Garrison.GetFollowers(123) is not a good idea and won't work as you expect.

That loop runs once to generate the available frames for followers, and that function only returns the followers you currently have, so when you gain new followers, you will get errors as there will not be enough frames available to use. You would have to reload the UI to recreate the frames with the new count each time you encounter that.

Just hardcoding the current max number is the simplest and easiest solution.

cheeseslicer commented 2 years ago

As far as the problems go with the out of wack window, after multiple hours of tinkering, I have resolved the issue completely to where even for i=1,#C_Garrison.GetFollowers(123) do will work beautifully. It is 5 different lines of code to slight tweak and its simply math numbers. Just search for the first parts of each line of code before the numbers. Numbers are all that needs to change. Below I have included the actual changes.

  1. self:SetHeight(98+72*math.ceil(#fl/4))
  2. t:SetText(FOLLOWERLIST_LABEL_TROOPS) t:SetPoint("TOPLEFT", 12, -20)
  3. s.troops[i]:SetPoint("TOPLEFT", (i-1)*68+14, -40)
  4. t = CreateObject("CommonHoverTooltip", CreateObject("InfoButton", f)) t:SetPoint("TOPRIGHT", -12, -20)
  5. for i=1,#C_Garrison.GetFollowers(123) do t = CreateObject("FollowerListButton", f, false) t:SetPoint("TOPLEFT", ((i-1)%4)68+14, -math.floor((i-1)/4)64-130) 1 2 3 4 5
N6REJ commented 2 years ago

@cheeseslicer your image is ambigous. I assume your talking about like 1960. Are you putting all that within the for loop? you can post your exact code by using the <> function in the menu here or by prefacing the text with 3 ` If you can supply the corrected code I'll add it to my repo which will contain the fully fixed file. https://github.com/N6REJ/VenturePlan2 the corrected file is in the "troopframe" branch

TruthNZ commented 2 years ago

The original licence with Venture Plan is "All rights reserved. Do not modify or redistribute." which prevents us making copies of it with modifications that others can access or use. And the Author has already shown he's willing to enforce that licence with legal backing. So I'd be really careful about that repo you've made.

Hence the lots of issues on this addon with explanations of how to modify the original yourself, instead of making files people can just download and use.

N6REJ commented 2 years ago

@TruthNZ he can't fight for a personal storage. but I understand. Honestly, idk that he can even legally say "do not modify". you mentioned he's tried legal means, do you have a reference just outa curiousity. I'm certainly not trying to cause any kind of drama here

woefulwabbit commented 2 years ago

@N6REJ "Do not modify" is probably not enforceable but for sure redistribution of the modified or unmodified code is enforceable. You can read more about what happened with attempts to fork VenturePlan here: https://www.reddit.com/r/wow/comments/ojqkgh/what_happened_to_the_community_venture_plan/ So ya, you probably should delete your repository.

One possibility though, is to publish a .patch file that can be applied to the original VenturePlan.

TruthNZ commented 2 years ago

Patch file is a really tempting idea - but does that fall under the same thing of redistributing modified code?

N6REJ commented 2 years ago

no, because its not part of the original code. I might have a solution really soon, apparently the copyright was only on the most recent version.

N6REJ commented 2 years ago

After a meeting with another user we are going to create a new extension forked from non-copyrighted version of VP. We welcome collaboration. Shadowlands Mission Helper This will solve all legal issues.

zealvurte commented 2 years ago

[...] apparently the copyright was only on the most recent version.

This is completely false. There has been a explicit "All rights reserved. Do not modify or redistribute." licence listed in the ToC file since 4.05. The absence of an explicit licence for earlier versions default them to all rights reserved. No version was placed in the public domain, and their copyright has not expired, so copyright still applies.

After a meeting with another user we are going to create a new extension forked from non-copyrighted version of VP.

What imaginary version do you believe was not only compatible with a FOSS licence, but exempt from copyright?

N6REJ commented 2 years ago

@zealvurte first lets cut the attack. I realize copyright law is very sensitive issue, but your mistaken in the fact that the absence of a copyright automatically creates such a copyright. So, we have to start @ 4.05 or earlier. Thats really not an issue is it?
If you actually check my repo you'd find that we are planning on a substantial change to how the extension works/looks not just a simple rename. The copyright "rights" you speak of have been tested in court several times and found to be invalid, to quote..

An implied license, if it exists, must, by definition, be non-exclusive because U.S. copyright law requires exclusive licenses to be in writing.

Should he attempt to imply some copyright enforcement I'll happily take it to court. I'm not even sure if foreign copyrights are enforceable in the US

N6REJ commented 2 years ago

also your impression of what is "public domain" and not is erroneous.. again to quote..

Public domain software is any software that has no legal, copyright or editing restrictions associated with it. It is free and open-source software that can be publicly modified, distributed or sold without any restrictions. SQLite, I2P and CERN httpd are popular examples of public domain software.

N6REJ commented 2 years ago

@cremor I agree copyright law is very confusing. As a FOSS member(?) for over 16yrs I've run into the copyright confusion many times. infact one of the organizations I work with has to keep 2 New York attorneys on retainer because of it.

zealvurte commented 2 years ago

first lets cut the attack.

I'm not sure where you think I attacked you. I stated facts unrelated to you, and asked you a question about it.

your mistaken in the fact that the absence of a copyright automatically creates such a copyright.

I'm not mistaken at all. Copyright is inherent and all rights are automatically reserved by an author unless they grant a licence otherwise. The quote you provided doesn't support what you claim in any way (the licence is implicit, non-exclusive, and not required to be written).

If you actually check my repo you'd find that we are planning on a substantial change to how the extension works/looks not just a simple rename.

Do you think modifying it somehow circumvents copyright or something, otherwise I don't understand why you bring this up.

also your impression of what is "public domain" and not is erroneous.. again to quote..

Nothing you have quoted is different to my accurate understanding of what public domain is. All I said is that no version has been put into the public domain (i.e. voluntarily waived the copyright).

I seriously question your experience with and understanding of copyright if you believe you have any legal rights to a copyrighted work just because it wasn't distributed with an explicit licence.

woefulwabbit commented 2 years ago

@N6REJ Most of the countries in the world are signatories of the Berne Convention, so copyright laws apply if you are in one of these countries and the work originates from another one of these countries.

The Berne Convention also states that all works are automatically copyrighted, without requiring registration or a copyright notice.

So whatever version of Venture Plan you intend to fork off is going to be copyrighted, which will make the fork a copyright violation.

N6REJ commented 2 years ago

@N6REJ Most of the relevant countries in the world are signatories of the Berne Convention, so copyright laws apply if you are in one of these countries and the work originates in another one of these countries.

The Berne Convention also states that all works are automatically copyrighted, without requiring registration or a copyright notice.

So whatever version of Venture Plan you intend to fork off is going to be copyrighted, which will make the fork a copyright violation.

Certainly not a copyright expert, however, I'm 99.9% sure that "substantial changes" are enough. For example covenant mission helper is the same basic software so to say we can't create our own extension doesn't make sense. Certainly not all the code would be unique. That would literally be impossible and would prevent any kind of libraries being used. I'm not trying to be a jerk.

N6REJ commented 2 years ago

@woefulwabbit The bottom line is I don't care for covenant mission helper and want to find/create a good replacement for VP. What we need to do to make that happen remains to be seen. Perhaps checking the issues we've come up with so far would help.

TruthNZ commented 2 years ago

Copyright is automatically applied to any creative work, without requiring the author to do anything. The author has to explicitly release the work into the public domain for a work to not have a copyright - or the copyright term has expired, currently 70 years after the author's death. This applies for all countries part of the Berne Convention, which is a majority of the world.

So unless you in Solomon Islands, South Sudan, Timor-Leste or Tuvalu (there might be others I've missed), the default all rights reserved style copyright applies, even without an explicit licence attached.

Now personally I want to see VenturePlan continued, or a good replacement addon (I don't think covenant mission helper does enough either).

I'm not trying to attack you, just saying you should be wary because the author has already shown he's not willing to share his code openly, thrown around DMCA notices and had Github and CurseForge enforce them (refer to the link above on what happened to previous attempts at a community version). I don't want to see another person with the intentions, the skills, and time (the bit I don't have) to develop a replacement, slapped down again.

N6REJ commented 2 years ago

@TruthNZ feedback in the new repo would certainly be welcome and helpful

cremor commented 2 years ago

Instead of arguing over copyright and licenses, why not contact the original author of VenturePlan and just ask for permission to take it over?

A few months ago there was a discussion on the WoWUIDev Discord and the original author said that he is willing to hand the addon over if he finds a person that he trusts to deliver quality work and not abandon the addon again soon. (Appearently there was even someone found, but since no new version was released I assume this didn't hold.)

N6REJ commented 2 years ago

@cremor I've done exactly that. Just waiting on response :)

k1ck3r commented 2 years ago

Because all of the changes you all guys published here, i had to deal with this issue like a simple user - helping a friend to fix his addon. Most of them are misleading because of "latest" changes posted in some other thread.

this post relates to VenturePlan itself and there is no direct relation with VenturePlanSoDMissions.

first of all, let's see how the table looks before (welcome page with not configured textures; troops and followers with example with full covenant upgrade, and low level one):

https://imgur.com/a/WDhUXd9

result should be (no text for troops/followers categories; repositioned the frame to fit BOTH covenant situations):

https://imgur.com/a/GM5hR0A

And because we don't want to publish a code which is currently unmaintained i'll just give it a go, and share a simple pastebin link where you can see my Widgest.lua - https://pastebin.com/Aek7e5Dk

As far as i can see, this repo VenturePlanSoDMissions is still unmaintained no matter that it had good start. Whoever wish to take it - contact author, if he doesn't answer - just clone the repo, make a PR, and if you are willing to share any info - find appropriate issue opened and let us all know. PRs are by far best way to keep contributors to followup on their notes/code changes. I hope you get, that this is not a hate or qq post, but i'm trying to ensure you that if a code is published as public everyone can clone it, and work on their own part of changes.

ps. my fixes are simple - commenting few lines where SetText and texture function usage, changed a bit math floor function - but that's it - nothing special

N6REJ commented 2 years ago

@k1ck3r I couldn't certainly fork it and merge pr's if they are created. Just let me know what you guys want to do. I'm willing to help where I can. I'm also willing ( and plan on ) creating a replacement for VP. Since there are copyright issues we'll have to "roll our own" but we can certainly use the concept and layout.
I plan on using ace3 frames anyway, not hard coded like he has done. But the visual layout/functionality I plan on being very close. I just need people to step up and help.

k1ck3r commented 2 years ago

Mate, just contact the author. If you did, but he didn't answer, try again. If you need someone to maintain the repo with code and the convo with author, i'll be glad to help. I think the only concern which could rise is, if you stop WoW or Blizz remove completely the table shit.

I'm not good coder (beeing sys/network admin for more than 30 years, and i know go, python, nodejs but at level which makes my day-to-day work easier) and can't offer strong support for the code. But i've been playing (paying sub for the whole time) WoW for almost 14 years, and i can only offer to support the repo and could do small fixes. But that's all from me :)

N6REJ commented 2 years ago

I've attempted the vp guy and I'll try again... I'm not horrendously worried about it at this point as we're going to most likely go with an entirely new engine.

gaveer commented 2 years ago

Because all of the changes you all guys published here, i had to deal with this issue like a simple user - helping a friend to fix his addon. Most of them are misleading because of "latest" changes posted in some other thread.

this post relates to VenturePlan itself and there is no direct relation with VenturePlanSoDMissions.

first of all, let's see how the table looks before (welcome page with not configured textures; troops and followers with example with full covenant upgrade, and low level one):

https://imgur.com/a/WDhUXd9

result should be (no text for troops/followers categories; repositioned the frame to fit BOTH covenant situations):

https://imgur.com/a/GM5hR0A

And because we don't want to publish a code which is currently unmaintained i'll just give it a go, and share a simple pastebin link where you can see my Widgest.lua - https://pastebin.com/Aek7e5Dk

As far as i can see, this repo VenturePlanSoDMissions is still unmaintained no matter that it had good start. Whoever wish to take it - contact author, if he doesn't answer - just clone the repo, make a PR, and if you are willing to share any info - find appropriate issue opened and let us all know. PRs are by far best way to keep contributors to followup on their notes/code changes. I hope you get, that this is not a hate or qq post, but i'm trying to ensure you that if a code is published as public everyone can clone it, and work on their own part of changes.

ps. my fixes are simple - commenting few lines where SetText and texture function usage, changed a bit math floor function - but that's it - nothing special

with the changes you made i can see new button with no function Screenshot_14