Foil rarity fix patch (completed)

[Seravy]

Aaaahh so that's what static does. Finally I understand. Wait, that's a very scary thing to do. I better stay away from it. Why would you ever want to do that, for that purpose, use a freaking global variable lol. It's counterintuitive to have a variable in an object that does not belong there.

...what does static do on a method, then? Because that sure was the reason why I had to add it.

[Agetian]

Aaaahh so that's what static does. Finally I understand. Wait, that's a very scary thing to do. I better stay away from it. Why would you ever want to do that, for that purpose, use a freaking global variable lol. It's counterintuitive to have a variable in an object that does not belong there.

...what does static do on a method, then? Because that sure was the reason why I had to add it.

Yeah, "static" can be a confusing thing, which is why many newer, "next-gen" languages such as Kotlin stay away from them. When used on a variable, it declares that the variable belongs to the class as a whole and not to the instance of the class (so, it's almost like a "global" variable inside that class). When used on a method, it declares that the method does not require a constructed instance of the class and can be used via a global call - ClassName.staticMethodName instead of creating an "object" instance of ClassName and then doing object.staticMethodName).

There are scenarios where they can indeed be useful. Forge uses them a lot in utility classes which are not meant to be instantiated in the first place (e.g. ComputerUtil*** are pretty much all static methods). Static methods can only use static variables, which is why you're facing an interesting dilemma right now as to how exactly to work around this particular design (as to why it was designed that way in the first place, I'm not sure, but I remember there were days when Forge did not have tabs and there could be only one deck editor open, maybe this particular design paradigm comes from back then? :/ )

- Agetian

[Seravy]

So these are basically your global variables and global procedures because going full OOP means you don't have them otherwise.
So...what's the point?
You could make a class "globalstuff", or put them into your main class, and call/reference it from there.
If you can't use instance local stuff inside in the first place, there is absolutely no reason for having them inside that class.

What kind of madman designed this language?

...more importantly, if you do not have a deck editor object, why do you need to put filters on it? this method makes no sense as being static, does it. I have to take a look.

[Agetian]

Yeah, check it out The "static" was rather commonplace in languages of the same era, including C++ which Java sort of used as inspiration for syntax and some of its elements.

Note that static variables/methods are not completely "global" as in "accessible from everywhere at all". They still belong to the class they're declared in, and they're referenced via that class's name. But yeah, you're right, inside the class they are sort of global because they don't belong to any particular instance.

There are some interesting considerations as to where and why they can be useful (in Java):
https://stackoverflow.com/questions/267 ... ic-methods

But like I said, most newer OOP languages try to stay away from "static" elements since they're too easy to abuse and too easy to break the rules of the OOP paradigm with

- Agetian

[Seravy]

Thanks, that actually had the solution among those comments!
"instead of calling new Util().method(arg), call Util.method(arg), or method(arg) with static imports. Easier, shorter."
Except, I do the reverse and then there is no need for static, yay?

...when I was making this code I was wondering...hey I haven't seen the spell shop create a manager. Oh, there is a different class for it. Wait, it's not a class related to CardManager? How does that even work, how come the shop still has the filter.
Well, now I know. Spell Shop is the guilty party that calls the CardManager without having a CardManager object. And that's the only thing why that has to be static.
The clean solution would probably be moving this method outside to its own subclass - If I ever have to do that for real, I'm totally going to name that class AntiStatic - but I'm going to be lazy for once and use the "new call" trick, as it was being called from only one location. (and now I actually have a way to specify the shop is a quest mode manager, nice!)

...success!

PS : Not sure if you noticed that edit so once more, to do test runs of 1000 packs, start a new quest and specify that set for the format and 1000 packs as your starting pool. Easy and you can use the deck editor to filter and count the cards.

[Agetian]

Ah, nice! Thanks a lot for looking into it! I'll look at the code soon!
And that's a handy little trick for generating lots of boosters

- Agetian

[Agetian]

Ok, testing the latest version. The fix for the filter in other modes definitely worked very well.

One more problem I just found: while in the deck itself, adding rating apparently does nothing (it works correct in the inventory). For example, try adding a few cards to the quest deck, then assigning ratings to the cards. Filtering by rating after that shows that all cards are still unrated.

- Agetian

[Seravy]

That's weird. I did try when I made that menu appear there and it worked. I still have the card I rated that way rated as 1. But now it isn't working anymore.
...oh, I think I get it. getCatalogManager will always get the inventory side, not the one the menu belongs to. I just got (un)lucky the same thing was selected on both sides so it worked.

Here, this works :

[Agetian]

Hmm, testing the foils generation again, and I think Masterpieces are no longer generated at all. Went ahead and generated 1000 packs of Amonkhet / Hour of Devastation, and then 1000 packs of Kaladesh / Aether Revolt, and didn't get a single one. This definitely used to work before, so it's introduced by your changes somewhere...

Here's the description of Masterpieces:
http://mtg.gamepedia.com/Masterpiece_Series

EDIT: The extra sheet was not accounted for in any way at all, I restored that and now the Masterpieces are generated again, but the chance is definitely too high since most likely the extra foil sheet does not take rarity of the original slot into account or something like that. Right now we're looking at 1 masterpiece per 250 cards or so, while in reality it should be considerably lower than that... Please help if you know how to improve the odds here to make it work better.

EDIT 2: Tried to improve the situation a bit by accounting for the rarity of the cards in the extra foil sheet and only adding those extra foils to the sheet of the appropriate rarity. This is better, but still off from the target values specified on that web page above. We're currently looking at the chances of 1 Masterpiece per 1200/1300 cards (1 in 83 boosters or so - a bit much) for KLD/AKH Masterpieces and 1 Expedition per 2000 cards (1 in 133 boosters or so) for BFZ Zendikar Expeditions (not sure how good/bad this is, actually, since Zendikar Expeditions chances are not directly specified on that page). This probably needs some tweaking but I'm not sure how to make it more authentic. :/

- Agetian

[Agetian]

Testing the rating patch - now setting rating in the deck window works correctly.

Shortcuts (0,1,2,3,4,5) don't do anything, it seems - Forge initiates full text search in the deck window and in the inventory window when you press those. Maybe they need to be something like Shift+1-5 or Alt+1-5 or something? :/

- Agetian

[Seravy]

Masterpieces huh.
I remember looking at that and it pretty much went like this :
"Zendikar doesn't even have an MPS sheet slot defined, it doesn't seem to be fully supported yet.
Maybe another set...yes it does seem to be in this one...except, that sheet is wrong? If there is one of each, how does that become a 1/144 packs chance? Ok, this isn't supported, I can ignore it."

Maybe I'm wrong but I believe what a foil sheet does is, it defines the chance by itself through having a different number of each card on it. So to make MPS work, you need to make the sheet contain the correct ratio of MPS to non MPS cards. Never seen that done by the code which is why I went with "I have no idea how this works, looks incomplete".

Let me see...yes, there is absolutely nothing there for the "chance" of cards, nor have ever been. As "extra foil sheet" is a generic thing, that's not surprising, if you want correct rates, you need to specify that, individually for each set. (unless it's magically the correct amount by picking 1 of each cards.) Assuming the system works on a specific rarity and not everything, you also need to specify the rarity, fortunately the system already does that - it only adds cards of rarities that appear in the sheet already.

I suggest the following modifications :
"ExtraSheetChance=0.694%", meaning the ratio is 1/144 packs (adjust this value for each set appropriately). Let's call this A. Let's call 1/A, the number of packs for one, B. This still assumes every pack is foil, that's not good. Then C=B*chance of foil in pack. This is how many FOIL packs you need for the desired extra sheet card.

This means there has to be 1/C foils from the sheet for (C-1)/C normal foils.
When building the sheet, first add the extra cards. This means you have to change code to look at the original sheet instead of the filled new one to determine the rarity.
Count the number. Multiply by C-1. Then add normal cards in a loop, one at a time, until C*(C-1) cards were added. Pick randomly, as the amount might be a faction of the available number of cards. It doesn't matter if the sheet contains different cards every time - we only pick one of them anyway, so the overall chance for each card is the same.

Then, you have a correct foil sheet, that gives you the correct percentages.

..but this assumes the foil sheet goes into every single rarity. If it only goes into the rare sheet, then you need to use D=C/6 instead of C to account for 5/6 foil packs not containing a rare.

Shortcuts...maybe you need to open the menu for those to work? idk, I never really used them, but I would think that's how it works. Otherwise, how would it know which card or even which side of the screen you are trying to work with?

PS : If we make this foil sheet thing work correctly, we'll be able to use it to support Planeshift alter-art foils by moving them from the set to a foil sheet.

[Agetian]

Ok, please let me know if you write the code for this to work with better ratio. I'll leave it in for now as is since it's closer than the original version was, and at least it works again (and wasn't working before with your modifications).

A big request btw: Please do not assume that something doesn't work or wasn't meant to work in Forge just because you think it doesn't/wasn't Please explore and test things before making conclusions. If something was implemented, it was implemented for some reason, and was meant to work. It may work correctly or incorrectly (like in this case, the chances are incorrect, so it's working but it isn't authentic), but it's a bad idea to knowingly break things completely that were working before, especially without asking the other devs whether something was working or not. That's not how we are approaching things, generally. If something breaks (and yes, things do break from time to time), we prioritize fixing it and potentially improving it, or reverting the commit that was responsible for the breakup, if fixing becomes impossible.

P.S. Will take a look at the shortcuts later today

- Agetian

[Seravy]

Okay, let's get a few things straight.

-When I posted this patch first, I specifically stated I know nothing about foil sheets, not even what they are meant to do, so it has to be tested by people who know - all I did was not touching code related to it. I don't remember revoking that statement, although my understanding of the stuff is better now than it was before. You can still find this in the first post of the thread.

-I did look into masterpieces. Zendikar is the first set with them. Zendikar.txt does not specify having an extra foil sheet, special slot, or any other means for them to appear. And, the set received an update related to that not very long ago, I remember that announcement. Yet, no foil sheet specified. That can only mean it's still incomplete. Planeshift, the other set with "always foil" extra cards, also isn't supported. It's a logical conclusion that extra "always foil" cards aren't supported (since then I realized the foil sheets are meant for exactly that, still neither ZEN nor Planshift had an extra foil sheet. The foil sheet code even had a warning comment that it is not fully functional...)

-Considering you introduced at least 3 new bugs in this patch trying to format things, at this point I won't accept "don't break things" unless I can be sure it was my fault - You didn't tell me what was wrong this time. Either way, not changing something and warning about it being untested in advance does not equal "intentionally breaking" things. I'm not sure I want to continue at this point. Your formatting obsession is driving me crazy, and now this. Formatting is nice but not important enough to break someone else's code for it or make him redo the whole testing process. Not to mention the first thing I was told was "For committing, generally we ask that you put together a patch file or two with your changes so we can test them out before we grant SVN access. " - one or two. We are probably over 10 by now. And while they had small problems, I don't think there was anything major or on a different level than what I run into when playing the game on a daily basis. Not to mention I always responded and fixed them as soon as I could. (well, ok, the 16th card in some packs was major but I did test drafts. Not idea why mine wasn't crashing.)

-This particular patch was an "emergency" bug fix patch. While making every single set work is obviously the final goal, quickly doing something about every set being wrong was what it intended to do. This was the problem that made me throw away 2 months of progress and start over my quest - we want to fix it ASAP so other people don't end up like that. I'm used to fixing severe bugs like this within a day to make sure as few players are negatively affected as possible. I'm shocked to see how slowly it was being handled here, with no sense of urgency whatsoever.

[Agetian]

Well, sorry to hear you're taking my words like that. First off, let's try to address the specific points you mentioned:

1. Yes, you mentioned that, but if I remember right you also specified that you did not touch the relevant code, so, according to you, it had to either work as it did before, or not work in case it didn't work before. This was not the case, since obviously you modified the relevant code (in particular, replaced extraFoilSheetKey with sheetKey in the conditional branch through which the Masterpieces were generated), which introduced the "masterpieces not generated at all" bug in the first place.

2. Zendikar (code ZEN) was not the first set with the Masterpieces, Battle for Zendikar was (code BFZ), which correctly specifies the extra foil sheet. In fact, for every set where ExtraFoilSheetKey is relevant (for the purpose for which it was originally implemented, at least) - the sets are BFZ, OGW, KLD, AER, AKH, HOU - it is correctly specified and used (and was working with the original code, albeit with the wrong generation chances). Therefore, you did not research that portion of code and/or period of Magic history well enough before making an assumption about whether it should work and how it should work.

3. Since you are writing and submitting a patch, it is your responsibility to ensure that it works correctly and is formatted according to the standards used in the project you're submitting to. If additional steps are needed before the patch is mainlined, it is also your responsibility to work in tandem with the reviewer who agreed to look into mainlining the patch to update it according to the coding standards used in the project and, if necessary, respond to the critique in order to work out the possible problems. Yep, I did break a few things (and I did warn you in advance that I could have broken things when I asked you to test), but at least the part of the reason why things broke so easily was because the original code as it was submitted was rather hard to read and partially to understand, for the most part because it was very poorly formatted, but also because the implementation logic/style was unfamiliar to me. Even if I didn't break that code and, as a result, we didn't test it and work the kinks out together so that it works *and* is properly formatted, someone else would have broken it sooner or later, since poorly written and poorly formatted code is difficult to work with. Moreover, not going to lie, I'm not the most experienced programmer on the team (and not a professional one), so someone else would have most likely done a better job integrating your work, potentially without necessarily breaking it. But, unfortunately for you, it looks like you'll have to make do with what I can offer you, at least for the time being, since no one else currently on the team appears to be eager to assume my role in reviewing and mainlining the patches. If anybody else would agree, I'd gladly relegate this role to a more experienced developer, after all, I have my own .todo list to follow as well. ^^

4. As for why you haven't yet been granted direct repository access - you can ask in #dev for details, but to cut a long story short, the team would need you to adapt to our coding standards (and, yes, formatting standards are a part of them too) and overall project vision. Coming in contact on Discord with the rest of the team regarding this would be a good start. I'm not going to speak for the entire team here - I'll just flag this post in #dev, maybe someone else would like to respond directly. And yes, Forge is not developed at a very rapid pace (it is, after all, a non-commercial, amateur, fan-made project), but the goal is not to do things as quickly as possible, it is rather to do them well enough and careful enough to hopefully provide an overall enjoyable experience to the players (and try to break as little as possible along the way - since, hey, most of us are non-professional programmers, so yes, we do break things from time to time too ).

At any rate, sorry to hear you're taking our approach and our collaborate work requirements with such hostility. Personally I hope to see you as our contributor (eventually with full access to the repository), but whether you continue to tinker with the code and attempt to make Forge better in the future or not is, of course, entirely your choice.

- Agetian

[Seravy]

(in particular, replaced extraFoilSheetKey with sheetKey

Okay, I checked my files and it definitely was my fault, it's already there in the first file. I can't imagine any reason for that change except accidentally deleting that line or part of it when moving things around and having to type it back.
That's unfortunate but wasn't intended.

The only modification I intended to do there was moving the IF some lines higher.

Look, as you yourself said, accidents happen. Problem is you accused me of breaking this "knowingly". That's not nice. Especially not after the first post saying "-As I don't know how foil sheets work, I haven't tested those, but the code for them is unchanged so it should not be affected. Please test."

It said "please test", you didn't.
Since I stopped playing, another 50 or 80 sets were released. Expecting me to be familiar with all of them immediately is unreasonable. As for the Battle for Zendikar not being Zendikar, I don't think naming like that ever happened in my era. I wasn't expecting that. Mercadian Nemesis is Nemesis. Mercadian Prophecy is Prophecy. Saviors of Kamigawa is Savoirs, etc.

I've used discord many times already, albeit usually asked about specific things. No one ever talked to me about "project vision" or how to become a dev, albeit I wasn't asking either, I assumed there will be a decision after a few patches and I'll get told "you're a dev now" or "you can't be a dev now for reason ...", or at the very least "do 3 more patches, your java still sucks" neither happened.

You seriously shouldn't have the wiki say "click here on the forum, wait for approval, and you're dev" when in reality it takes a lot more than that. Misleading.

I definitely will continue to tinker with the code (or not, based on previous experience, working a game takes forever and it's generally a better idea to just play something else.) - but I'm tempted to take the easy path for a change and keep it to myself, if it means so much extra trouble to have it added. That frees up your time too, I noticed it's inconvenient for both of us.

As for speed, this isn't some "still in alpha, unreleased" project. Taking time in adding features, and making sure the quality is high is common sense. But it is also common sense to fix severe bugs immediately. And I say this not as a developer, but as a player of the game. Maybe the problem here is my obsession with foil cards, I consider this a much more severe problem than most would.