Splitting CheckInvPaste() into smaller functions

[matheusgomes28]

Moved functions that were getting previous items into smaller ones with better names. At least you can see straight-away what each of these blocks of code do!

This has been sitting on my github for a few weeks, just got back to this today and realise the timedemo test is failing. Will fix that, but creating this to get some feedback anyway.

[matheusgomes28]

I am struggling a little bit to debug the timedemo_test, as I get the following errors when running it locally:

ERROR: Asset not found: lua\user.lua
Demo file not found
...

I can't find user.lua anywhere, am I missing something?

[StephenCWills]

Are you debugging with Visual Studio?

EDIT: If so, try this...

---

Here's the text for that second image.

{
  "version": "0.2.1",
  "defaults": {},
  "configurations": [
    {
      "type": "default",
      "project": "CMakeLists.txt",
      "projectTarget": "devilutionx.exe",
      "name": "devilutionx.exe",
      "args": []
    },
    {
        "type": "default",
        "project": "CMakeLists.txt",
        "projectTarget": "devilutionx.exe",
        "name": "timedemo",
        "args": [
            "--config-dir",
            "${cmake.buildRoot}\\test\\fixtures\\timedemo\\WarriorLevel1to2",
            "--save-dir",
            "${cmake.buildRoot}\\test\\fixtures\\timedemo\\WarriorLevel1to2",
            "--diablo",
            "--spawn",
            "--lang",
            "en",
            "--demo",
            "0",
            "--timedemo"
        ]
    }
  ]
}

And you may have to select Build > Build All from the toolbar to make sure the test fixtures get copied to the build directory. That's probably why you're seeing Demo file not found. FYI, user.lua has nothing to do with it.

[matheusgomes28]

Thanks for the reply, @StephenCWills .

I am running on VsCode with the MSVC compilers on Windows, and the clang compilers on Ubuntu. I figured what the issue I have is: it's simply that we get the prefPath in paths.cpp from the SDL call to SDL_GetBasePath() which get's where the binary is located. Given my following launch.json in VScode:

        {
            "name": "Timedemo Debug",
            "type": "cppvsdbg",
            "request": "launch",
            "program": "${workspaceFolder}\\build\\vs2022-deb\\Debug\\timedemo_test.exe",
            "cwd": "${workspaceFolder}\\build\\vs2022-deb\\",
            "args": [
                "--config-dir",
                "C:\\Users\\mathe\\development\\devilutionX\\build\\vs2022-deb\\test\\fixtures\\timedemo\\WarriorLevel1to2",
                "--save-dir",
                "C:\\Users\\mathe\\development\\devilutionX\\build\\vs2022-deb\\test\\fixtures\\timedemo\\WarriorLevel1to2",
                "--diablo",
                "--spawn",
                "--lang",
                "en",
                "--demo",
                "0",
                "--timedemo"
            ],
            "stopAtEntry": false,
            "environment": [],
            "console": "integratedTerminal"
        }

SDL_GetBasePath() will return ${workspaceFolder}\\build\\vs2022-deb\\Debug as the base path, and we later add on the paths text/fixture/... to that in order to create the path to demo_0.dmo. This works fine when you have ctest integration in your IDE (like visual studio, clion, or simply running ctest) because CMake seems to copy the bin as if it was called from the directory where add_test(...) was called from.

In my case, I endup with the demo_0.dmo path as C:\Users\mathe\development\devilutionX\build\vs2022-deb\Debug\/test/fixtures/timedemo/WarriorLevel1to2\demo_0.dmo

I do wonder: should we not use the value of the current working directory as the base path, instead of the path to the binary? Either way, I haven't fixed it yet, and the solution seems to just run the test through the test explorer and let ctest do the magic.

[StephenCWills]

SDL_GetBasePath() will return ${workspaceFolder}\\build\\vs2022-deb\\Debug as the base path, and we later add on the paths text/fixture/... to that in order to create the path to demo_0.dmo. This works fine when you have ctest integration in your IDE (like visual studio, clion, or simply running ctest) because CMake seems to copy the bin as if it was called from the directory where add_test(...) was called from.

Since we don't use add_test(), at least not directly, then we'll have to go by the documentation for gtest_discover_tests() instead.

devilutionX/test/CMakeLists.txt

Lines 51 to 52 in bc4f434

	add_executable(${test_target} "${test_target}.cpp")
	gtest_discover_tests(${test_target})

That said, it seems the WORKING_DIRECTORY property of that call works the same.

https://cmake.org/cmake/help/latest/module/GoogleTest.html#command:gtest_discover_tests

Specifies the directory in which to run the discovered test cases. If this option is not provided, the current binary directory is used.

---

I do wonder: should we not use the value of the current working directory as the base path, instead of the path to the binary? Either way, I haven't fixed it yet, and the solution seems to just run the test through the test explorer and let ctest do the magic.

We do use CMAKE_CURRENT_BINARY_DIR by default as the base path to deploy the test fixtures.

devilutionX/test/Fixtures.cmake

Line 99 in bc4f434

set(dst "${DEVILUTIONX_TEST_FIXTURES_OUTPUT_DIRECTORY}/${fixture}")

devilutionX/test/Fixtures.cmake

Lines 1 to 3 in bc4f434

	if(NOT DEFINED DEVILUTIONX_TEST_FIXTURES_OUTPUT_DIRECTORY)
	set(DEVILUTIONX_TEST_FIXTURES_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/fixtures")
	endif()

That path should match the one gtest_discover_tests() is using as its WORKING_DIRECTORY. Since the executable is deployed to CMAKE_RUNTIME_OUTPUT_DIRECTORY, I do agree that it would make more sense if we used a relative path in the test exectuable instead.

However, I'm not really that familiar with ctest. It looks to me like CMake is generating a bunch of files in the format *_test[1]_tests.cmake that lists the WORKING_DIRECTORY property. This is presumably the working directory that CMake uses to execute the test. I'm not sure what Visual Studio, CLion, or ctest would use. Or does the bootstrapper within the test executable itself set up its own working directory before executing the test? 🤷

---

Anyway, if you want to give it a shot, here's where we set pref path up to use relative paths in pfile_write_hero.

devilutionX/test/writehero_test.cpp

Line 372 in bc4f434

paths::SetPrefPath(".");

[matheusgomes28]

@StephenCWills

Since we don't use add_test(), at least not directly, then we'll have to go by the documentation for gtest_discover_tests() instead.

If you look at the source code for gtest_discover_tests(), seems to be pretty much a wrapper around add_test() with some extra google test specific stuff, hence the same behaviour.

I kind of gave up trying to fix the prefix paths outside the ctest integration and just hardcoded the correct pathPrefix(...) in the test so I can debug. I'm pretty sure ctest changes the binary path itself by either copying the binary to the WORKING_DIR or some other way, then the SDL calls get the binary path dir, not the current working dir as the base directory.

Either way, I think I fixed the tests... turns out I removed one too many if checks on item locations.

[AJenbo]

Looks good over all, this is a truly massive function so hard to tackle. It's fine if you do it in steps and just extract some of the obvious bits first.

[matheusgomes28]

Thanks @AJenbo . I'll removed some of the comments I just realised I left, then you are free to merge this.

[kphoenix137]

Perhaps a better variable name? This is quite ambiguous

[matheusgomes28]

I agree, hence why this feedback request! I guess it gets the item ID from the inv grid? I was not too sure on this one.

[AJenbo]

Looks like prevItemId would have been a good name since that reflects the getter function name and this is the returned value.

[kphoenix137]

Also a quite ambiguous variable name

[AJenbo]

Nice this is a good enough clean up that the function is now manageable to read 👍

I'll go over it in detail soon.

[AJenbo]

would be nice to use the same break style now that the switch isn't so bloated.

[AJenbo]

It would be nice if these new functions for changing equipment took there arguments in the same order: Player, Slot, Other, rather then being a bit random about it.

[AJenbo]

I don't really have anything against std::optional, but only positive values are valid returns from this so we can use -1 to indicate that nothing was found which allows a bit more direct use with out the need to check before passing the value on to the next user.

Some thing more relevant for this function and refactoring is that if you flip the if statements you can get rid of the else cases and instead make use of early returns. This takes a lot fewer lines and avoids the pyramid structure of the nested conditions.

This also reveals something odd in that CheckOverlappingItems() is always feed a 0 as the 4th argument as this value is never set by anything before it's called.

int8_t GetPrevItemId(int slot, const Player &player, const Size &itemSize)
{
	if (player.HoldItem._itype != ItemType::Gold)
		return CheckOverlappingItems(slot, player, itemSize, 0);
	int8_t item_cell_begin = player.InvGrid[slot - SLOTXY_INV_FIRST];
	if (item_cell_begin == 0)
		return 0;
	if (item_cell_begin <= 0)
		return -item_cell_begin;
	if (player.InvList[item_cell_begin - 1]._itype != ItemType::Gold)
		return item_cell_begin;
	return 0;
}

[AJenbo]

When running this PR I'm unable to place items in the belt using the mouse. Could you please look in to what is causing this?

It seems to be swallowed by this bit of code

	if ((il != ILOC_UNEQUIPABLE) && (desired_loc != il))
		return;

[AJenbo]

Previously there was and else in between these, with out it it is now not possible to place items on the belt as desired_loc is never ILOC_BELT.

Suggested change

	}
	if ((il != ILOC_UNEQUIPABLE) && (desired_loc != il)) {
	} else if ((il != ILOC_UNEQUIPABLE) && (desired_loc != il)) {

[AJenbo]

I applied a few changes and did a bit further cleanups, hope you are ok with me doing so. Overall a good clean up, thanks for putting time an effort in to it.