Cataclysm-dda: rollerskates and rollerblades do nothing

[curstwist]

Game version:7436

Operating system:windows 10

Tiles or curses:tiles

Mods active:filthy clothing, disable npc needs, simplified nutrition

Expected behavior:

rollerskates and blades should reduce movement cost when on hard surfaces

Actual behavior:

they don't decrease speed at all and often raise it by a point or so. Walking I get 95/100 movement values. Running I get 95/50 using either footware. For comparison, boots are 95/90 walking and 95/45 running. Grass has the same values.

Steps to reproduce the behavior:

start a world, debug some skates and try walking/running on a hard surface.

[DracoGriffin]

Can confirm with a fresh character that has 120 base speed on road, only changes to 116 base speed for both rollerblades and rollerskates. If they are refitted, the base speed drops to 111. This occurs on any type of terrain -- it doesn't seem to be exclusive as intended.

player.cpp line 1880:

    }
    if( worn_with_flag( "ROLLER_INLINE" ) ) {
        if( on_road ) {
            movecost *= 0.5f;
        } else {
            movecost *= 1.5f;
        }
    }
    // Quad skates might be more stable than inlines,
    // but that also translates into a slower speed when on good surfaces.
    if( worn_with_flag( "ROLLER_QUAD" ) ) {
        if( on_road ) {
            movecost *= 0.7f;
        } else {
            movecost *= 1.3f;
        }
    }

Looking on the CDDA subreddit, looks like there were a few1 reports2 on rollerblades3 not working a month back -- if that information helps anyone track down the issue.

[tyler-beshearse]

Extra info on bug: Rollerblades will stop working as soon as a world has been re-loaded once. They will work when playing a newly created world for the first time. Behavior is easy to reproduce: make a new world as a Skater Boy and note your move speed. Save/Quit/Load and they won't work anymore. It's not tied to character either, making a new character in an previously played world will not fix it.

Build: 7448

[noyoupickaname]

The last time roller skates/blades worked properly was in 3c8f96a with it breaking in be23065 on April the 10th.

The listed changes are all json files and obviously not the cause. Comparing the two revealed a vast number of changes between them which were not listed at all in the commit notice. I found no difference between the entries for roller skates and roller blades, and a few changes between movecost but none of those look like it would be the cause. I'm assuming that the problem is a change in savegame.cpp, however upon looking through the changes in it I don't see anything that stands out to me. Having said that savegame is a bit of a mess to put it nicely so I might be missing something there.

Hopefully knowing exactly where it breaks will help someone else figure out what the problem is. Especially given that its been two and a half months now.

EDIT: Accidently put the wrong date. It broke on April the 10th, not the 1st.

[noyoupickaname]

Sorry about the second post but I'm new to GitHub and I just realized you can page someone. So... @BorkBorkGoesTheCode halp! I'm assuming you probably have the best chance of figuring out what when wrong here given that the changes are in a pr of yours.

[BorkBorkGoesTheCode]

That is a level of voodoo I don't understand. @noyoupickaname what are the changes you see on your end? @Rivet-the-Zombie @narc0tiq @kevingranade what could be going on here?

I've just used git diff to compare the commits in #23447. If that's possible I'm going to stop making PRs to the project. I apologize for being a burden.

[narc0tiq]

3c8f96a with it breaking in be23065
Comparing the two revealed a vast number of changes between them which were not listed at all in the commit notice

The two commits aren't ancestors of each other -- they're independent pull requests. There are differences between them based on the fact that they started from different parents (I think these are 8614ebc9eeb800d9442450aebd237fe29a0a50dc and 8f58f37a3a6b0b430d1ed7afad1b631f724197a4, respectively, but this is actually quite tricky to follow), but there's no reason to believe the specific two commits are the cause.

[noyoupickaname]

If that's possible I'm going to stop making PRs to the project. I apologize for being a burden.

Hold on @BorkBorkGoesTheCode . Lets back up for a minute here. I don't see any reason for you to stop making PRs and I do not think you are being a burden. I don't believe anyone else here things that either and if the do... well screw them. There are who knows how many people working on the project and there is over 56k commits. Sooner or later I'm sure just about everyone has introduced some sort of issue and if everyone left after that no one would be working here. I wasn't meaning for it to sound like I was trying to call you out or anything like that either. In fact if the only changes you did are what's listed in that PR then I'm certain those are not the cause. Its just that for some reason there are other changes in that PR that aren't listed and I don't know what's going on. Just thought it might be helpful if you could look at this and that you might be able to help given the situation, that's all.

Seriously, don't go.

The two commits aren't ancestors of each other -- they're independent pull requests. There are differences between them based on the fact that they started from different parents (I think these are 8614ebc and 8f58f37, respectively, but this is actually quite tricky to follow), but there's no reason to believe the specific two commits are the cause.

@narc0tiq I have no idea what the cause is, thus why I tried to get someone to help here. All I do know is that rollerblades/skates worked properly for 3c8f96a and then the very next commit of be23065 is when the behavior in this thread started. As I said before the changes between the two listed on here are only in the json files and I don't see how that would be the issues. In fact before posting here the first time I copy and pasted the entire json folder from 3c8f96a to be23065 before compiling (Although I don't see why it would change anything if it was done after compiling, but just in case.) to make absolutely sure and the bug still occurred. Obviously this means there must have been other changes so I broke out WinMerge and the list of changes is massive. I'm not even sure where to start here.

I'm new to GitHub but yeah, I've quickly realized that it can be very difficult to find out what is descendant from what. Now that you post it however is everyone complete sure that what was removed in 8f58f37 was actually unused? I'm not able to check it right now but I'll try adding what was removed back in later and seeing if that fixed it.

In all honesty in addition to being new to GitHub I'm still learning C++. I just saw that there was a bug that no one was sure when exactly it popped up and no one seemed to really be working on so I thought I'd try and help. At the least I could narrow down when it happened and that's what I did. If anyone else is in doubt go ahead and download the two, compile, and see how it works. Then compare the difference between the source for the two.

[Rivet-the-Zombie]

If that's possible I'm going to stop making PRs to the project. I apologize for being a burden.

Geeze Louise, please don't do that! We value your contributions, and you've done nothing wrong.

[noyoupickaname]

Well trying to revert 8f58f37 didn't work either as it refused to compile. If anybody has a commit they want me to check, or a bunch of them they want me to plow through looking for a certain behavior, just let me know. It only takes me about 2 min for me to compile so I can go through those pretty fast. Other than that I'm useless here now.

Geeze Louise, please don't do that! We value your contributions, and you've done nothing wrong.

Seriously. I have no idea what the problem is, I just know that it wasn't anything listed in what you've done. I know that for certain because I reverted all the changes and the behavior persisted so it absolutely wasn't that. Its just that somehow, someway, the behavior was introduced at that time in something else. It wasn't you. And even if that wasn't the case and it had been you, which it wasn't, everyone would still be telling you to stay.

[noyoupickaname]

Found the source of the problem. In worldfactory.cpp changing:

if( elem.second.getDefaultText() != "" && !elem.second.is_hidden() ) {

back to:

if( elem.second.getDefaultText() != "" ) {

completely fixes the issue. The context its currently in is:

if (!is_conversion) {

    const auto savefile = world->folder_path() + "/" + FILENAMES["worldoptions"];

    const bool saved = write_to_file( savefile, [&]( std::ostream &fout ) {

        JsonOut jout( fout );



        jout.start_array();



        for( auto &elem : world->WORLD_OPTIONS ) {

            // Skip hidden option because it is set by mod and should not be saved

            if( elem.second.getDefaultText() != "" && !elem.second.is_hidden() ) {

                jout.start_object();



                jout.member( "info", elem.second.getTooltip() );

                jout.member( "default", elem.second.getDefaultText( false ) );

                jout.member( "name", elem.first );

                jout.member( "value", elem.second.getValue() );



                jout.end_object();

            }

        }



        jout.end_array();

    }, _( "world data" ) );

    if( !saved ) {

        return false;

    }

}

I've tested it on the current master and notice nothing odd going on with saving a game or with reloading a save. As far as I can tell no mods even use the external_option that this was written for and even if any did I don't think this change would cause a problem. Having said that I'm not going to issue a PR yet as I would like for someone more knowledgeable about the code base to look at it and make sure there isn't a problem I'm missing here. @Firestorm01X2 any thoughts?

[noyoupickaname]

So, separating the other issue I found into a second post...

There seems to have been a problem with version control that I ran into here. As noted earlier, be23065 had several hundred changes between it and the master right before it. Normally I would assume that this is some sort of one off error and just one of those things that occasionally happens except it doesn't appear to be an isolated incident.

While looking for the issue that caused this bug going back much further I noticed that it popped up shortly only to disappear again at least four times. Each time that it happened (excluding the initial commit that added it) it was added back in along with a massive number of other changes. What I'm guessing happened here, and someone correct me if I'm wrong, is the people were using older branches to work on or that a PR had waited for a while. Either way new changes were added into the project and then when that PR happened it copied their entire fork and not just the changes that were meant to be implemented. Someone catches it, and then adds back everything that was reverted. Something like that seems to have been the case with be23065 which is when the changes that added this bug finally became permanent. I get that something like this can happen but you really should notify the community if you are going to be adding hundreds of changes at once, even if you are simply reverting back to what was already in there.

I should also note it seems clear that @BorkBorkGoesTheCode was completely unaware that a vast number of changes were being added back in with his work on be23065 . I don't know if this was an innocent issue with version control or if someone intentionally added them back during that commit.

TL:RD: Version control seems to have been horrible, at least during the period this was going on (I hope that its been fixed.) and nobody bothered to let the community know. Thus why a large number of unlisted changes seem to happen at once only to shortly be gone again, and then the cycle repeats.

PS: Can someone talk to @BorkBorkGoesTheCode and let him know that none of this at all appears to be his fault? If I knew how to get ahold of him I would. @Rivet-the-Zombie @narc0tiq and @kevingranade I'm under the impression that he at least respects your work here and that's why he asked for your input here earlier. So if any of you can talk to him. Also I know he helps maintain a mod with @DangerNoodle so maybe you could talk to him.

[Firestorm01X2]

@noyoupickaname
Yes.
I've made changes that affects saves.
https://github.com/CleverRaven/Cataclysm-DDA/pull/23226

1. Now options with flag COPT_ALWAYS_HIDE no longer save. It should help to prevent situation, explained there: #22769 (comment).

Now options with flag COPT_ALWAYS_HIDE

All HIDDEN options. Not external only.
External options are just hidden options that was added dinamically from json.
I did it intentionally.

Maybe this should looked at. Even if it should be safe and it was tested by me, I've expected discussion about it.

But it was just merged.

If it is it, then it is my fault.

[Firestorm01X2]

Look at this:
https://github.com/CleverRaven/Cataclysm-DDA/pull/23226/files

That changes:

//Skip hidden option because it is set by mod and should not be saved
                if ( opt.hide == COPT_ALWAYS_HIDE ) {
                  continue;
          }

   // Skip hidden option because it is set by mod and should not be saved
                if( elem.second.getDefaultText() != "" && !elem.second.is_hidden() ) {
                     jout.start_object();

But anyway. How exactly it affects rollerblades?

As far as I can tell no mods even use the external_option that this was written for and even if any did I don't think this change would cause a problem.

But there is 2 (two exactly) external option in base game now.

Your change do not break external options and hidden options, I suppose. But it make then go to saves that I specially want to prevent.

[ZhilkinSerg]

Here is the thing - following function does not return true when player is wearing rollerblades or rollerskates:

https://github.com/CleverRaven/Cataclysm-DDA/blob/539bc31370ebb74084347257345e90712e4ae068/src/character.cpp#L1174-L1179

In current version of game unwear and drop all your character clothes, debug spawn kimono and rollerblades (do not wear yet) and run following statements in Lua console:

print("wearing FANCY"..tostring(player:worn_with_flag("FANCY"))
print("wearing ROLLER_INLINE"..tostring(player:worn_with_flag("ROLLER_INLINE"))`
print("wearing ROLLER_QUAD"..tostring(player:worn_with_flag("ROLLER_QUAD"))`

You will get results:

wearing FANCY:true
wearing ROLLER_INLINE:false
wearing ROLLER_QUAD:false

Wear kimono and run same statements - you get:

wearing FANCY:true
wearing ROLLER_INLINE:false
wearing ROLLER_QUAD:false

Now wear rollerblades and run same statements - you get:

wearing FANCY:true
wearing ROLLER_INLINE:false
wearing ROLLER_QUAD:false

Save game, go to \data\json\items\armor.json, remove FANCY flag and add ROLLER_QUAD flag to kimono definition and load saved game.

Run same statements - you get:

wearing FANCY:false
wearing ROLLER_INLINE:false
wearing ROLLER_QUAD:true

Unwear and drop kimono, then run same statements - you get:

wearing FANCY:false
wearing ROLLER_INLINE:false
wearing ROLLER_QUAD:false

That means no flags are returned for rollerskates and rollerblades.

---

Edit: I believe #23316 could be related and if I am correct more things could be broken down.

[narc0tiq]

@noyoupickaname

Version control seems to have been horrible, at least during the period this was going on

I think it may be a matter of methodology rather than content. The CDDA repository is not a perfect linear history, despite what github may show in its commit list -- this repo, like many others, is constantly branching and merging as new PRs are created and accepted.

I have a representative sample graph¹ of the last 700-ish commits as of this writing, ordered chronologically according to the commit creation date (which is what github shows as the "commit history"). What should be visible is that branches exist independently of each other _until merged_, at which point the entire content of the merge source appears in the merge target, regardless of its chronological position.

This also means that if you compare across branches, you'll see more differences than actually exist, because you're also comparing two different starting points from the canonical master -- for example:

• 9ac6f4c8e9 and bf914c4a54 are ~4h apart,
• the former is started from 966bfea979, which is just 3 hours earlier,
• but the latter is actually started from 36376e4f7c, about 39 commits (or 2-ish days) earlier,
• during which a great many other branches were merged, which are included in 9ac6f4c8e9, but not in bf914c4a54,
  
  
  
  • even though bf914c4a54 is more recent.
  
  
• In other words, if you check out bf914c4a54 and compare it to 9ac6f4c8e9, you would get a diff including all the changes from 36376e4f7c to 966bfea979, even though these are "not included" in either of the branches we're comparing.

To help in determining the true starting point of an issue, git supplies the git bisect command, which understands the history as it really is (as a directed acyclic graph) rather than a linear list. Given two commits as starting points (one where the issue does not occur, and a later one where the issue does), git bisect will help you perform a graph-aware binary search where you only need to tell it, for each candidate it offers, whether it has the problem or not.

So, given that you have the ability to compile Cata yourself, try a git bisect to narrow down where the issue started occurring. It really is very useful to have this data, and we appreciate your good will and desire to help.

---

Footnotes:
¹ - the graph is created using http://bit-booster.com/graph.html, to which I gave a copypasta of a few hundred lines from a more complete log.

[ZhilkinSerg]

Ha, the definitions which are actually used for rollerblades and rollerskates come from legacy data file (data/legacy/4/boots.json) instead of (data/json/items/armor/boots.json`). Legacy file does not contain necessary flags for speed increase.

https://github.com/CleverRaven/Cataclysm-DDA/blob/539bc31370ebb74084347257345e90712e4ae068/data/legacy/4/boots.json#L663-L723

[Zireael07]

Why are legacy definitions used?

[ZhilkinSerg]

Why are legacy definitions used?

Long story short, because of CORE_VERSION world option.

---

World option CORE_VERSION controls what migrations are applied for legacy worlds

When new world is created, value of this world options is set to current value of core_version (6).

https://github.com/CleverRaven/Cataclysm-DDA/blob/c0f46810f65ec1e372f79e3261d2da06cea4b2f3/src/game.cpp#L142

https://github.com/CleverRaven/Cataclysm-DDA/blob/c0f46810f65ec1e372f79e3261d2da06cea4b2f3/src/options.cpp#L1417-L1421

When world is loaded its value is read from saved world options and all patches which are less than current value of core_version are applied:

https://github.com/CleverRaven/Cataclysm-DDA/blob/c0f46810f65ec1e372f79e3261d2da06cea4b2f3/src/game.cpp#L3786-L3790

This option is hidden (COPT_ALWAYS_HIDE), but #23226 made changes not to save such options, therefore once world is saved and reloaded, game tries to load value of option CORE_VERSION, but fails and defaults it to 0.

---

I believe we need to separate conditions for displaying and saving of world options.

[ZhilkinSerg]

So I guess I've nailed this one in #24160. Please test.

[noyoupickaname]

So I guess I've nailed this one in #24160. Please test

I suppose a little late given that its already been merged but I tested it and everything seems to work fine. If there is any specific behavior you're concerned about let me know and I'll see if I can find anything.

So, given that you have the ability to compile Cata yourself...

Barely. I've been unable to get a successful compile in Solus. In Windows MinGW refuses to do anything other than spit out errors regardless of what I do. Oddly enough Codeblocks works using MinGW, however Codeblocks is hit and miss with SDL working maybe one fifth of the time and Lua never working. This has been the worst program to compile I've ever worked with. If I can manage to get a process that works reliably for either OS I'll update the compiling guide here. I saw that @ZhilkinSerg has updated the guide for MSYS2 so I'll try that. It should also be pointed out that going through the threads on Discord on compiling were of no help at all. Actually there were several instances of people just berating someone for not using Linux or of Kevin locking the thread. TBH I'm not entirely surprised at that as there seem to be some toxic issues within the community (Obviously this doesn't apply to everyone but enough that its an issue.) and thus why I have no interest in anything other than bug fixes that no one else is working on and compiling help here.

The rest of your comment is... strange. I noted issues with version control in the master and your response is to go into detail about what could be a textbook explanation of version control issues, even informing me there is a tool to keep track of the version control problems. I don't even... I'm not sure if you don't see why some would consider it a problem or if its just a "whatever" kind of thing. Either way, doesn't matter I guess. If you guys don't care then alright.

[ZhilkinSerg]

I've started with MSYS2 as it came up the simpliest configuration. Will eventually update MinGW and CodeBlocks guides.

[narc0tiq]

This has been the worst program to compile I've ever worked with

I agree, it's really difficult with all the dependencies -- I don't even try to compile Windows builds on Windows. Thank goodness for mxe and osxcross.

and your response is to go into detail about what could be a textbook explanation

Thank you for calling it textbook, but I clearly failed to get across the point that it's not broken, it's complicated. Projects with more than one or two contributors almost always have histories that are difficult to understand, especially if you ignore branches, and Cata has had 640 contributors over its lifetime (41 over the last month).

I mentioned this because you seem to be relying on Github's view of a linear history, best exemplified by your mention of _be23065 and the master right before it_, because depending on where you look, those can be different commits:

• looking at the commit history for master, we see 3c8f96a immediately before it: https://github.com/CleverRaven/Cataclysm-DDA/commits/master?after=f3e70f1fbca7e9f8753db299558909bc854a08bf+1425
• looking at the commit history for be23065 alone, we see 8f58f37 as immediately prior -- this is the actual parent commit: https://github.com/CleverRaven/Cataclysm-DDA/commits/be23065106bd57b111543d0b55273433d9f2e584

So the difference here comes from Github showing a linear view of master, flattening the actual branches down into a sort-of-chronological view of when each commit was created. We can know that chronologically @BorkBorkGoesTheCode first created 3c8f96a, then made be23065, but we don't see that these two commits are in different branches -- respectively, the branch called heli_group used for #23325, and the branch called rifle_clips used for #23447.

By comparing 3c8f96a and be23065, we're also comparing their different branch-start points from master (i.e., comparing 8614ebc9 to 8f58f37a), so of course it looks like a large difference: https://github.com/CleverRaven/Cataclysm-DDA/compare/8614ebc9...8f58f37a.

I'm not sure if you don't see why some would consider it a problem

I do see it, but it's -- as I said -- a matter of methodology. You can only understand a directed acyclic graph as a line if the graph actually has no branch points. Otherwise, you need to consider what information you actually want (i.e., where between these two commits did this thing break?) in terms of the graph (which of these 379 commits introduced the bug?).

ETA:

If you guys don't care then alright.

If I didn't care, I would've stopped with my first comment and not spent several hours of my time trying to explain the obvious point of confusion, but I believed that you wanted to help so I wanted to give you the tools to do so.

[BorkBorkGoesTheCode]

@noyoupickaname @Rivet-the-Zombie @narc0tiq @ZhilkinSerg I've read your comments and know I didn't cause the bug. Due to some stuff happening in my life recently I won't be contributing. Thanks for the good times.

[ZhilkinSerg]

@BorkBorkGoesTheCode, sure, that was not you. Good luck in your endeavors!

[Rivet-the-Zombie]

Long days and pleasant nights to you, @BorkBorkGoesTheCode.