Logic error in MagicPattern.GetPathLevels()

[geepope]

GetPathLevels() is used (mostly indirectly) in a variety of places to get the theoretical maximum paths from a magic pattern, potentially including randoms. However, there is a logic error that prevents it from parsing multiple randoms correctly: the variable prob is initialized before looping through each random but is not reset between randoms. Because prob is multiplied by a value that is almost certainly lower than 1, prob is lowered on each successive random which makes it harder and harder to pass the given probability. In particular, most calls to GetPathLevels() that are looking for randoms will pass a probability parameter of 0.25 which means that a single 100% 4-element random will pass but any subsequent randoms will automatically fail. Because of this, nationgen will look at a mage with a 1?1 pattern and a mage with a 1?3 (unlinked) pattern and "see" that they have the same maximum levels with randoms even though they very obviously do not.

The fix for this is obvious - it would be simple to remove prob altogether and just compare chance to the probability threshold instead. However, the apparent pointlessness of prob raises a question as to whether this behavior was somehow intentional: there is a certain logic to saying that multiple 100% randoms are unlikely to land on the same element and should be disregarded by the same logic that 10% randoms would be ignored, although this leads to fairly absurd conclusions when you get to the 1?3 unlinked pattern. More generally speaking, this method gets called from a lot of places and although the majority of mages would return the same results regardless fixing the bug could lead to unanticipated behaviors.

			    double prob = 1;
			    for(RandomEntry e : randoms)
			    {
				
				if(Generic.containsBitmask(e.paths, masks[i]))
				{
					// Realamount = actual increase in path
					// At amount <= 1 it is 1
					// At amount > 1 (ie linked randoms) it is amount
					double realamount = 1;
				    double amount = e.amount;
					
				    
					if(amount > 1)
					{
						realamount = amount;
						amount = 1;
					}
					double chance = amount / (double)Generic.AmountOfVariables(e.paths);


					prob *= chance;		
					

					if(prob < probability)
						continue;
					else
						rand += realamount;
				
				}
			}

[elmokki]

I think what you describe at the end is the original function of the method when I wrote it a long time ago: If you call it with 50%, it should tell what paths you get at 50% probability. No documentation, so I assume the literal method name is what I wrote it to do.

That said, there may be a very good case for changing the method or making a separate method just like you describe. I guess it really depends on where it is used exactly. Especially the 1?3 - both linked and unlinked - might result in weird outcomes if the argument probability is too high. I think the concept of the method when called is to get what recruiting the mage is likely to result in with different likelihoods, and it may indeed fail in certain cases.

[geepope]

Yeah, I was concerned about those edge cases but in practice it might not actually matter. Most existing calls are either looking for no randoms or >25% randoms; if there were paths with more exotic randoms this could cause issues but even triple unlinked randoms won't break the 25% threshold on anything that would be missed so it's probably moot. I was a little alarmed initially by the number of places it gets called but now that I've got more time to look at them it looks like a lot of them are unused methods and most of them ignore randoms anyhow. A more robust probability calculation could theoretically be useful at some point but I don't think I need it at present.

[geepope]

Now that I'm looking, there is at least 1 pattern that breaks the method's assumptions on a 25% probability call; there's a quite rare pattern that's got a 4-element 100% random and two unlinked 2-element 100% randoms, has noticeably better than 25% chance at 2+ of the same path but will still get counted as only 1. Not exactly a show-stopper, though, and in a pinch it would still pass the quick-and-dirty version of GetPathLevels() if the randoms were reordered so that the 4-element one was at the bottom.