Bug Gelatinous Noob absorbed enchantments persist through logout

Phillammon

New member
To reproduce:
With two accounts in Gelatinous Noob, A and B. Log in to A. Absorb 15 x Light that Never goes Out. Log in to B. Absorb 15x meat paste. Modtrace Smithsness.

Expected behaviour:
B reports 0 Smithsness
Actual behaviour:
B reports 75 Smithsness from absorbed equipment.
 

lostcalpolydude

Developer
Staff member
What are you trying to accomplish with resetModifiers()? I suspect that everything you are hoping for is already in the reset() call above that.
 

fronobulax

Developer
Staff member
If you log out of A and then log into B do you see the same unexpected behavior?

Over the years I have seen cases where I am logged into A and then log into B and some things KoLmafia reports for B are incorrect for B but not for A. This has never been repeatable and I have no realistic idea of a cause unless there is a Java Virtual Machine feature I don't know about but perhaps you have discovered something repeatable?
 

fredg1

Member
What are you trying to accomplish with resetModifiers()? I suspect that everything you are hoping for is already in the reset() call above that.

Modifiers.java:
Java:
public class Modifiers
{
    private static final Map<String, Object> modifiersByName = new HashMap<>();
    private static final Map<String,String> familiarEffectByName = new HashMap<>();
    private static final Map<String,Integer> modifierIndicesByName = new HashMap<>();
    private static final List<UseSkillRequest> passiveSkills = new ArrayList<>();
    private static final List synergies = new ArrayList();
    private static final List<String> mutexes = new ArrayList<>();
    private static final Map<String,Set<String>> uniques = new HashMap<>();
    public static String currentLocation = "";
    public static String currentZone = "";
    public static String currentEnvironment = "";
    public static double currentML = 4.0;
    public static String currentFamiliar = "";
    public static String mainhandClass = "";
    public static double hoboPower = 0.0;
    public static double smithsness = 0.0;
    public static double currentWeight = 0.0;
    public static boolean unarmed = false;

    // [...]

    private String name;
    public boolean variable;
    private final double[] doubles;
    private final int[] bitmaps;
    private final String[] strings;
    private ModifierExpression[] expressions;
    // These are used for Steely-Eyed Squint and so on
    private final double[] extras;
    public Modifiers()
    {
        this.variable = false;
        this.doubles = new double[ Modifiers.DOUBLE_MODIFIERS ];
        this.bitmaps = new int[ Modifiers.BITMAP_MODIFIERS ];
        this.strings = new String[ Modifiers.STRING_MODIFIERS ];
        this.extras = new double[ Modifiers.DOUBLE_MODIFIERS ];
        this.reset();
    }

    // [...]
    
    public final void reset()
    {
        Arrays.fill( this.doubles, 0.0 );
        Arrays.fill( this.bitmaps, 0 );
        Arrays.fill( this.strings, "" );
        this.expressions = null;
    }
    
    // [...]
    
    public static void resetModifiers()
    {
        Modifiers.modifiersByName.clear();
        Modifiers.familiarEffectByName.clear();
        Modifiers.passiveSkills.clear();
        Modifiers.synergies.clear();
        Modifiers.mutexes.clear();
        Modifiers.uniques.clear();
        Arrays.fill( Modifiers.bitmapMasks, 1 );

        BufferedReader reader = FileUtilities.getVersionedReader( "modifiers.txt", KoLConstants.MODIFIERS_VERSION );
        String[] data;
        
        // [reads from the file...]
    }
    
    // [...]

    public static final void overrideModifier( String lookup, Object value )
    {
        if ( value != null )
        {
            Modifiers.modifiersByName.put( lookup, value );
        }
        else
        {
            Modifiers.modifiersByName.remove( lookup );
        }
    }
}

CharPaneRequest.java:

Java:
public class CharPaneRequest
    extends GenericRequest
{
    // [...]
    
    private static void checkAbsorbs( final String responseText )
    {
        if ( !KoLCharacter.inNoobcore() )
        {
            return;
        }
        
        // [parses absorb information from the character page...]
        
        Modifiers.overrideModifier( "Generated:Enchantments Absorbed", modList.toString() );
    }
    
    // [...]
}


TL;DR:
Code:
KoLCharacter.currentModifiers.reset();
resets the *non-static* fields. However, (for some reason), *these are not the only fields that get modified*.

As seen on the CharPaneRequest excerpt, the *static* modifiers are also frequently modified during adventuring, but they are *only* reset when using the moon-spoon in TCRS, while they should actually also be reset on ascension and on logout.

I really don't get why the "new" modifiers (like the "Generated:Enchantments Absorbed" from the excerpt above) aren't stored in the Modifiers object of KoLCharacter, but Modifiers.java is such a huge, disgusting mess/tangle that I don't really intend to try and find out (and Gausie who claimed that making more and more of mafia be data-driven was the good thing to do, because "it's the mafia way"...).


A source of confusion may have come from the fact that phil should technically have used
Code:
Modifiers.resetModifiers();
instead of
Code:
KoLCharacter.currentModifiers.resetModifiers();
, since it's a static method.
 

Phillammon

New member
What are you trying to accomplish with resetModifiers()? I suspect that everything you are hoping for is already in the reset() call above that.
I had initially hoped for that as well, but as has been detailed by fredg above, this is not the case- Reset only resets the double, string and bitmap arrays, and leaves the modifiersbyname map untouched, which is the relevant one. (It should be noted that I have tested the patch noted above and it does indeed repair this behaviour)

This said, there is a potential argument to be made that the resetModifiers() call should be moved inside reset so reset works how you think it does, but that feels dangerous for reasons I am struggling to articulate. Happy to be told otherwise by devs.
If you log out of A and then log into B do you see the same unexpected behavior?

Over the years I have seen cases where I am logged into A and then log into B and some things KoLmafia reports for B are incorrect for B but not for A. This has never been repeatable and I have no realistic idea of a cause unless there is a Java Virtual Machine feature I don't know about but perhaps you have discovered something repeatable?
Fronobulax, yes, that is the behaviour I am describing- this is indeed the cause of that, if I am understanding you correctly.
 

heeheehee

Developer
Staff member
This said, there is a potential argument to be made that the resetModifiers() call should be moved inside reset so reset works how you think it does, but that feels dangerous for reasons I am struggling to articulate. Happy to be told otherwise by devs.
My impression from reading this thread is that resetModifiers() would bring a performance penalty (since we'd need to repopulate modifiers by reading modifiers.txt, rather than relying on the cache). My concern is that said performance penalty would be most visible in places where we create lots of new Modifiers objects, like, um, Maximizer.

It doesn't seem too bad if we're only incurring it once per login (since presumably you don't have hundreds of thousands of accounts you're working through every day).

As seen on the CharPaneRequest excerpt, the *static* modifiers are also frequently modified during adventuring, but they are *only* reset when using the moon-spoon in TCRS, while they should actually also be reset on ascension and on logout.

I really don't get why the "new" modifiers (like the "Generated:Enchantments Absorbed" from the excerpt above) aren't stored in the Modifiers object of KoLCharacter, but Modifiers.java is such a huge, disgusting mess/tangle that I don't really intend to try and find out (and Gausie who claimed that making more and more of mafia be data-driven was the good thing to do, because "it's the mafia way"...).
So, a couple of notes here.

TCRSDatabase.resetModifiers() has been called in refreshSessionData and on liberateKing (since r19256), so I'm not sure what your first point is getting at.

If we stored generated modifiers in the Modifiers object, we'd need to copy them for each speculation when using Maximizer. KoLCharacter.currentModifiers is not the only Modifiers object in existence.

Regarding code being data-driven: when done correctly, it scales much more readily than adding new cyclomatic complexity for every new case you want to support. (Not to say there's no complexity -- when you start putting things in datafiles that don't belong together, understanding how all the components interact adds complexity in itself.) That doesn't mean there's no room for improvement, but describing the code as a "a huge, disgusting mess/tangle" without offering solutions detracts from the conversation and quite frankly makes others less willing to engage with you.
 

heeheehee

Developer
Staff member
Discussing the actual patch at hand, and comparing with the handling for TCRS:

Correct me if I'm wrong (I've only skimmed the patch, not applied it): this adds a couple of lines to KoLCharacter.reset(). From grepping over the codebase, this is called in a number of places:

- LoginManager (presumably, logging in...)
- LogoutManager (logging out...)
- ValhallaManager (onAscension)
- KoLCharacter (overload which takes a String username as opposed to a boolean newCharacter)

This seems like it misses the case of liberating the king.

(I have no objection to calling resetModifiers indiscriminately in any of these cases, as it seems safe and simpler than adding new hardcoding for every similar path down the line.)
 

fredg1

Member
Regarding code being data-driven: when done correctly, it scales much more readily than adding new cyclomatic complexity for every new case you want to support. (Not to say there's no complexity -- when you start putting things in datafiles that don't belong together, understanding how all the components interact adds complexity in itself.) That doesn't mean there's no room for improvement, but describing the code as a "a huge, disgusting mess/tangle" without offering solutions detracts from the conversation and quite frankly makes others less willing to engage with you.
...sorry.


My main gripe is how Modifiers.java is 5k lines long, with undocumented fields and methods all on the same level, and the "Object" class often being used to store data across arrays/lists.
Using classes and sub-classes to store data and organize methods would help greatly, IMO, but from my recent experience...
From my recent experience, doing this would have required too big of a change for it to be accepted anyway, so I kind of... I kind of rejected the idea before even saying it (while keeping the complaint that came with it)...

I apologize...
 

heeheehee

Developer
Staff member
From my recent experience, doing this would have required too big of a change for it to be accepted anyway
I think part of the problem is that you're thinking too big. You've described several distinct refactors that could be split into manageable patches. Just to list a few: adding documentation, logically grouping functionality (which lends itself to moving code into separate files), adding structure to reduce the use of polymorphic lists.

I hope you understand where I'm coming from. Big patches take a lot of time and effort to review. There's a lot more that can go wrong in the review process. And if the patch gets rejected, you've wasted a ton of effort with nothing to show for it.
 

fredg1

Member
I hope you understand where I'm coming from. Big patches take a lot of time and effort to review. There's a lot more that can go wrong in the review process. And if the patch gets rejected, you've wasted a ton of effort with nothing to show for it.
I definitely understand how and why it helps, but I have issue with this practice for both personal, social and specific-to-this-situation/project - related issues.

(I'm listing them in order to either get help solving them, or to get leniency in understanding my struggles to apply this method)

Global -
It's possible to notice an issue only once you're well underway of something else. What do you do once you notice it? If the issue stops you from continuing what you're doing, you can't just ignore it? But do you just discard what you did? Also, is the issue even fixable in the code's current state? (said like this, it just comes out as an organization problem, but it comes into play further ahead)

Personal -
I have this blessing/curse called hyperfocus. This leads to two things.
1- When I notice an issue, my attention may shift over that issue. That probability increases with the issue's prevalence (e.g. is it stopping you dead in your tracks? Does it just need some temporary workaround?).
This is made worst by the fact that my brain doesn't have any kind of "filter". Common people can ignore/filter out the voice of other people, or noise, when trying to focus (up to a certain point, of course). I can't. No matter what I do, if I hear something, my brain will (attempt to, "spent resources to") process what that is, no matter how debilitating to my current situation this is. It's the same thing with thoughts. If this issue pops up often/has a wide repercussion, I can't just "ignore" it.
2- I can't select what I hyperfocus on, and it's pretty much always on "something". This means that if I lay something down, there's little to no chance of me coming back to it, because I'll be focused on something else, and would need to be focused on something "related"/"close" to it to give my hyperfocus a chance to latch back on that thing.
This exacerbates the "Global" problem mentioned above. If I just discard my current work to temporarily work on the issue, I just won't be interested in the original work anymore...
If I wait to fix the issue until I finished the originally planned work, I'll have forgotten about the issue...

Project-related - KoLmafia.us and submitting patches
Multiple issues come to mind.
1- Some mafia developers have openly admitted to having a "If it ain't broke, don't fix it" mentality. What do you do if one of the "smaller changes" you could submit on its own doesn't actually have any negative impact in the code's current state, but becomes a problem in what you're trying to do?
Splitting big changes into smaller ones (plural) is a good idea in general, but in those cases, what do you do if you can't make the case for each of these smaller changes, individually? Especially if you submit them *before* the big change that relies upon this/these smaller change(s) is finished, so you can't immediately show how the fix is helping?
2- continuing the previous entry, in a way; submitting a patch means waiting for it to be approved or rejected (or asked for something to be changed, questions, etc.) (and with timezones, that can means things going as slow as 1 post per day). What do you do meanwhile? I can't just "wait", without touching the rest of the project, I'll lose hyperfocus! What do I do if the individual fix is rejected? What if there's no answer? Do I just continue the bigger project that relies on it anyway?

Project-related - using Git (more like a nitpick than an actual obstacle to using this method)
The fact that we're using Git means that we have to submit .patch files that submit the whole change in a single step.
Using github could help in showing the individual steps used to achieve a goal without having to submit a different patch each time.
 

Phillammon

New member
Discussing the actual patch at hand, and comparing with the handling for TCRS:

Correct me if I'm wrong (I've only skimmed the patch, not applied it): this adds a couple of lines to KoLCharacter.reset(). From grepping over the codebase, this is called in a number of places:

- LoginManager (presumably, logging in...)
- LogoutManager (logging out...)
- ValhallaManager (onAscension)
- KoLCharacter (overload which takes a String username as opposed to a boolean newCharacter)

This seems like it misses the case of liberating the king.

(I have no objection to calling resetModifiers indiscriminately in any of these cases, as it seems safe and simpler than adding new hardcoding for every similar path down the line.)
That's correct- if you so desire I will happily add it to liberateKing too. At present absorbed enchantments are ignored when not in noobcore regardless, which solves this indirectly, but futureproofing seems like a wise idea.
 

heeheehee

Developer
Staff member
That's correct- if you so desire I will happily add it to liberateKing too. At present absorbed enchantments are ignored when not in noobcore regardless, which solves this indirectly, but futureproofing seems like a wise idea.
Yes please! I'd like to express our intent explicitly when possible, and as mentioned, I wouldn't think this is a big enough performance hit to matter. If you really want, leave a comment describing that it's unnecessary but why you're doing it anyways, for the next person who reads this code.

1- Some mafia developers have openly admitted to having a "If it ain't broke, don't fix it" mentality. What do you do if one of the "smaller changes" you could submit on its own doesn't actually have any negative impact in the code's current state, but becomes a problem in what you're trying to do?
Splitting big changes into smaller ones (plural) is a good idea in general, but in those cases, what do you do if you can't make the case for each of these smaller changes, individually? Especially if you submit them *before* the big change that relies upon this/these smaller change(s) is finished, so you can't immediately show how the fix is helping?
One concern that I think we devs generally share is that we're always worried about backsliding / breaking existing functionality. This is generally exacerbated by our historical lack of reproducible, committed tests. I would be much happier to review large changes if there were adequate tests in place to help convince me that they're safe.

If one of those refactors is critical to what you want to do, but it's not clear why it's useful, then we've possibly skipped a few communication steps in the middle, namely: is your end-goal actually desirable, and is there a different way of going about it? On the other hand, for the refactors I described above, each change has tangible benefits from safety and clarity perspectives. But those are moot if correctness concerns come into play.

I recognize that it's challenging to make lots of refactors that touch the same file and build upon each other. If you're using git (with git-svn), this is absolutely an opportunity to create a local feature branch, split it up into multiple self-contained commits that you can submit for review one at a time, and keep working on your side branch, so you can then shelve it for later as we work through the review process. Shepherding code through the review process is as crucial, if not more so, than the act of writing code in the first place.

One other thing I find valuable in distributed software development is going back and curating my changes such that they're easier to review, often by minimizing the diff by removing irrelevant changes. Like, if you want to go in and clang-format a whole directory, okay, I guess... that's easy enough to review that the only changes are whitespace (but even so, please don't send a 5000-line patch and expect us to get around to it quickly!).

2- continuing the previous entry, in a way; submitting a patch means waiting for it to be approved or rejected (or asked for something to be changed, questions, etc.) (and with timezones, that can means things going as slow as 1 post per day). What do you do meanwhile? I can't just "wait", without touching the rest of the project, I'll lose hyperfocus! What do I do if the individual fix is rejected? What if there's no answer? Do I just continue the bigger project that relies on it anyway?
See above re: feature branches with git, or hg, or whatever other DVCS you want to use.

Project-related - using Git (more like a nitpick than an actual obstacle to using this method)
The fact that we're using Git means that we have to submit .patch files that submit the whole change in a single step.
Using github could help in showing the individual steps used to achieve a goal without having to submit a different patch each time.
Nitpick to your nitpick: we're using subversion, centrally-hosted on Sourceforge. There are ongoing internal discussions re: moving to git, centrally hosted on Github. That doesn't stop you from using local feature branches via git-svn or HgSubversion.

I disagree that using github means that we submit large changes as atomic units. Yes, you can create large pull requests, but even so, reviewers still run into the same problems of "this thing is several thousand lines of code, how do I even begin to review this" and "I don't agree with all of these changes."
 

fredg1

Member
I disagree that using github means that we submit large changes as atomic units. Yes, you can create large pull requests, but even so, reviewers still run into the same problems of "this thing is several thousand lines of code, how do I even begin to review this" and "I don't agree with all of these changes."
My point here was that, at least, with github, the various commits that constitute a pull requests show the changes *starting at the end previous commit*.
With git/subversion, if you split a big change into smaller steps, you need to wait for each submitted step to be accepted and added to the project before you can submit the next one. Otherwise, the .patch file will *include* the previous step in it (if the change is in the same file as the previous one(s)), undoing the benefits of splitting your changes into smaller ones (unless there is a feature to avoid this that I did not know about. Feel free to inform me about this).
 

heeheehee

Developer
Staff member
This in part comes back to my point about communication.

With github, if you propose a large pull request, you've still done all the work up-front, without discussing whether this feature is desirable, much less how you envision getting there. On the other hand, if you've spent some time upfront talking about what you want to do, and how, then you can outline the change that you're making and how it fits into the big picture. We don't necessarily need to see all the code upfront to understand what you're doing and why.

Do I think there's benefit in improved patch tracking + management? Absolutely, that's a large part of the discussion around changing development platforms. But it's not a silver bullet. Even if we move to github, I can't imagine there's much appetite among the dev team for reviewing 5000-line pull requests.

Re: splitting a change: you're not limited to just one feature branch, especially locally, where branches are cheap :)

In all seriousness, if you have sufficiently self-contained patches, then even if you're touching the same files, you should be able to apply them independently. Yes, some changes will require others to be merged upstream first. But you shouldn't use that as an excuse to create 5-, 10-, or even 20-commit chains which must be applied sequentially.

And as mentioned, from a pragmatic viewpoint, if your patches are smaller, they're easier to review, less likely to be considered objectionable, and faster to get accepted upstream.
 
Top