Add leather addon #1720

ottml · 2024-12-01T16:32:53Z

Flamefire

Thanks for working on this! I added some inline notes

Flamefire · 2024-12-01T16:54:05Z

libs/s25main/Loader.cpp

+                            carrierEnum = fat ? leatheraddon::BobTypes::FAT_CARRIER_CARRYING_SKINS :
+                                                leatheraddon::BobTypes::THIN_CARRIER_CARRYING_SKINS;
+
+                        if(ware == GoodType::Leather)


Those should be else-ifs for better readability

Flamefire · 2024-12-01T17:01:44Z

libs/s25main/Loader.cpp

@@ -512,6 +521,9 @@ bool Loader::LoadFilesAtGame(const std::string& mapGfxPath, bool isWinterGFX, co
    if(!LoadResources({"wine_bobs"}))
        return false;

+    if(!LoadResources({"leather_bobs"}))


Is it possible to condition those on the enabled addons? Feels wasteful but maybe the remaining code can't handle those to not be present?

The idea was also to integrate those with the "regular" files and have them be loaded automatically if the addon is enable (see the above TODO and initResourceFolders (addon argument)). Do you see any sensible archive for that?

Flamefire · 2024-12-01T17:04:45Z

libs/s25main/Loader.cpp

@@ -907,6 +920,24 @@ void Loader::fillCaches()
                          wineaddon::bobIndex[fat ? wineaddon::BobTypes::FAT_CARRIER_CARRYING_WINE :
                                                    wineaddon::BobTypes::THIN_CARRIER_CARRYING_WINE]
                          + static_cast<unsigned>(imgDir))));
+                    } else if(leatheraddon::isLeatherAddonGoodType(ware))
+                    {
+                        leatheraddon::BobTypes carrierEnum;


Could this be an extra function (leather ware->leather bob index)? I'd also suggest an assertion as it currently isn't directly clear, that carrierEnum will always be initialized

Flamefire · 2024-12-01T17:05:28Z

libs/s25main/SerializedGameData.cpp

@@ -102,6 +105,7 @@
 /// 9: Drop serialization of node BQ
 /// 10: troop_limits state introduced to military buildings
 /// 11:: wineaddon added, three new building types and two new goods
+/// 12:: leatheraddon added, three new building types and three new goods


Increase version below too

Added compat code and increased version :)

Flamefire · 2024-12-01T17:08:18Z

libs/s25main/desktops/dskCredits.cpp

@@ -358,7 +359,7 @@ void dskCredits::DrawCredit()
 template<typename T>
 T randEnum()
 {
-    return T(rand() % (helpers::NumEnumValues_v<T> - 2));
+    return T(rand() % (helpers::NumEnumValues_v<T> - 5));


This looks odd (did before with the -2). Where does this magic number come from? Why does it apply to all random enum values generated? It is e.g. used for Job but then wine & leather is checked.for in line 404

Refactored the drawing code to be more clear and readable.

Flamefire · 2024-12-01T17:10:58Z

libs/s25main/factories/JobFactory.cpp

@@ -179,6 +182,15 @@ std::unique_ptr<noFigure> JobFactory::CreateJob(const Job job_id, const MapPoint
        case Job::TempleServant:
            RTTR_Assert(dynamic_cast<nobUsual*>(goal));
            return std::make_unique<nofTempleServant>(pt, player, static_cast<nobUsual*>(goal));
+        case Job::Skinner:
+            RTTR_Assert(dynamic_cast<nobUsual*>(goal));


all that C&P...
Maybe a function for the assert-dynamic_cast-then-static_cast would be useful? Can't think of a good name though

Flamefire · 2024-12-01T17:13:36Z

libs/s25main/figures/nofLeatherWorker.h

+    void DrawWorking(DrawPoint drawPt) override;
+    /// Id in jobs.bob or carrier.bob when carrying a ware
+    [[noreturn]] unsigned short GetCarryID() const override;
+    /// Der Arbeiter erzeugt eine Ware


I'd not add any more German docstrings. Here it doesn't even add more information than the name of the function already does, so just remove

Suggested change

/// Der Arbeiter erzeugt eine Ware

Flamefire · 2024-12-01T17:21:26Z

libs/s25main/ingameWindows/iwBuildings.cpp

@@ -91,7 +95,7 @@ iwBuildings::iwBuildings(GameWorldView& gwv, GameCommandFactory& gcFactory)

    // "Help" button
    Extent btSize = Extent(30, 32);
-    AddImageButton(35, GetFullSize() - DrawPoint(14, 20) - btSize, btSize, TextureColor::Grey,
+    AddImageButton(38, GetFullSize() - DrawPoint(14, 20) - btSize, btSize, TextureColor::Grey,


can you add a named constant for this (and possibly other) IDs? I.e. a file-scoped anonymous enum.
It might also be better to put all the building buttons last (with an offset) such that we don't need to "guess" the ID here.

Flamefire · 2024-12-01T17:21:46Z

libs/s25main/ingameWindows/iwShip.cpp

+                leatheraddon::BobTypes type = leatheraddon::BobTypes::SKINNER_WALKING;
+                if(job == Job::Tanner)
+                    type = leatheraddon::BobTypes::TANNER_WALKING;
+                if(job == Job::LeatherWorker)


Suggested change

if(job == Job::LeatherWorker)

else if(job == Job::LeatherWorker)

libs/s25main/gameData/BuildingConsts.cpp

…ly died our died by the hunter)

…naturally died our died by the hunter) fix clang tidy

…s for coins - Order, upgrade, enable/disable - Add armor member to nofSoldier, reduce first armor before hitpoints

- refactor gui to handle adding new stuff more easily

…lding as for coins fix wrong logic c++ code

add missing comp code for new ham distribution category

…nd version to 2

code review

temporary fix we clean this up later

aztimh · 2024-12-10T05:01:36Z

libs/s25main/gameData/BuildingConsts.cpp

@@ -352,6 +364,9 @@ const helpers::MultiEnumArray<SmokeConst, Nation, BuildingType> BUILDING_SMOKE_C
    vikings[BuildingType::DonkeyBreeder] = SmokeConst(4, {-27, -40});
    vikings[BuildingType::Vineyard] = SmokeConst(1, {18, -48});
    vikings[BuildingType::Winery] = SmokeConst(1, {-14, -32});
+    vikings[BuildingType::Skinner] = SmokeConst(1, {7, -39});


I made a typo with this one: it should be: vikings[BuildingType::Skinner] = SmokeConst(1, {-7, -39});

Use correct index for fat armor carrier 280 instead of 282, which is wrong in docu

…naturally died our died by the hunter) Add check so skinner do not skinn the same animal, which leads to crash

aztimh · 2024-12-11T07:26:28Z

libs/s25main/gameData/DoorConsts.h

   // Römer
-   {4, 6,  8,  0,  12, 8,  16, 4, 0,  -3, 8, 8, 8, 8, 6,  0, 10, 12,  14, 12,
-    9, 12, 12, 16, 19, 14, 16, 0, -8, 17, 0, 6, 9, 8, 14, 6, 4,  -13, -8, 2},
+   {4, 6,  8,  11, 12, 8,  16, 4, 8,  -3, 8, 8, 8, 8, 6,  12, 10, 12,  14, 12,


So after some testing I think the Roman Leatherworks doorconst should be 8 not 12.

aztimh · 2024-12-11T10:46:47Z

libs/s25main/gameData/BuildingConsts.h

-   {{{0, 0},    {19, -4},  {19, -3},  {0, 0},    {23, -19}, {22, -7},  {-8, -10}, {9, -24},   {0, 0},     {29, -23},
-     {-2, -15}, {2, -13},  {-5, -16}, {-5, -15}, {0, 0},    {0, 0},    {0, 0},    {4, -16},   {9, -12},   {7, -10},
+   {{{0, 0},    {19, -4},  {19, -3},  {-14, -1}, {23, -19}, {22, -7},  {-8, -10}, {9, -24},   {-16, -10}, {29, -23},
+     {-2, -15}, {2, -13},  {-5, -16}, {-5, -15}, {0, 0},    {22, 6},   {0, 0},    {4, -16},   {9, -12},   {7, -10},


I made another typo. African leatherworks X sign offset should be: {22,-6}

aztimh · 2024-12-11T10:47:24Z

libs/s25main/gameData/BuildingConsts.h

-   {{{0, 0},    {19, -6},  {19, -20}, {0, 0},    {5, -15},  {-32, -20}, {4, -16}, {10, -18},  {0, 0},    {20, -10},
-     {15, -10}, {15, -10}, {15, -10}, {15, -10}, {0, 0},    {0, 0},     {13, -5}, {-5, -13},  {15, -20}, {15, -1},
+   {{{0, 0},    {19, -6},  {19, -20}, {-7, -3},  {5, -15},  {-32, -20}, {4, -16}, {10, -18},  {3, -18},  {20, -10},
+     {15, -10}, {15, -10}, {15, -10}, {15, -10}, {0, 0},    {5, -4},    {13, -5}, {-5, -13},  {15, -20}, {15, -1},


And Babylonian Leatherworks X sign should be: {15,-31}

…lding as for coins Use corretc x sign offsets

Change different offsets and animations

…lding as for coins Invert upgrade order for armor. Now same as for gold

Use function for convertion and add invalid enum type

Refactor credits for more readable code

Fix clang tidy

Use enums for buttons and add buttons without offsets first

Adding ware workflow, buildings and menus for leather addon

2b08623

Flamefire reviewed Dec 1, 2024

View reviewed changes

ottml added 5 commits December 1, 2024 18:30

Fix formatting

327aba1

Add distribution tab for ham and make gui more general

56790b6

Add compatibilty code

131f464

Add direct transport route

dddc893

Add fixed animations from leather_bobs.lst

94f5625

aztimh reviewed Dec 5, 2024

View reviewed changes

libs/s25main/gameData/BuildingConsts.cpp Outdated Show resolved Hide resolved

ottml added 11 commits December 7, 2024 15:57

Skinner goes out to get skins from carcass if he has no pigs (natural…

4c45d10

…ly died our died by the hunter)

LeatherAddon military (1): Add SetArmorAllowed game command

245cb1b

fixup! Skinner goes out to get skins from carcass if he has no pigs (…

f0adbcf

…naturally died our died by the hunter) fix clang tidy

LeatherAddon military (2): Add logic for armor in military building a…

4173c08

…s for coins - Order, upgrade, enable/disable - Add armor member to nofSoldier, reduce first armor before hitpoints

LeatherAddon military (3): Add armor gui elements to military building

264b5d6

- refactor gui to handle adding new stuff more easily

fixup! LeatherAddon military (2): Add logic for armor in military bui…

817e3c1

…lding as for coins fix wrong logic c++ code

Add test for SetArmorAllowed

727adb9

fixup! Add compatibilty code

c0546ba

add missing comp code for new ham distribution category

Add compatibility code for ChangeDistribution and increase game comma…

70485bb

…nd version to 2

fixup! Adding ware workflow, buildings and menus for leather addon

fb8639d

code review

fixup! Adding ware workflow, buildings and menus for leather addon

ea18d5f

temporary fix we clean this up later

aztimh reviewed Dec 10, 2024

View reviewed changes

ottml added 2 commits December 10, 2024 19:40

fixup! Add fixed animations from leather_bobs.lst

0340c20

Use correct index for fat armor carrier 280 instead of 282, which is wrong in docu

fixup! Skinner goes out to get skins from carcass if he has no pigs (…

55c25c8

…naturally died our died by the hunter) Add check so skinner do not skinn the same animal, which leads to crash

aztimh reviewed Dec 11, 2024

View reviewed changes

ottml added 5 commits December 11, 2024 16:42

fixup! LeatherAddon military (2): Add logic for armor in military bui…

0ceb01c

…lding as for coins Use corretc x sign offsets

fixup! Adding ware workflow, buildings and menus for leather addon

592231a

Change different offsets and animations

fixup! LeatherAddon military (2): Add logic for armor in military bui…

1e7cea9

…lding as for coins Invert upgrade order for armor. Now same as for gold

fixup! Add fixed animations from leather_bobs.lst

f5db3a7

Use function for convertion and add invalid enum type

fixup! Adding ware workflow, buildings and menus for leather addon

de07029

Refactor credits for more readable code

ottml added 2 commits December 14, 2024 14:15

fixup! Add fixed animations from leather_bobs.lst

8d77cd3

Fix clang tidy

fixup! Adding ware workflow, buildings and menus for leather addon

1b8743f

Use enums for buttons and add buttons without offsets first

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add leather addon #1720

Add leather addon #1720

ottml commented Dec 1, 2024

Flamefire left a comment

Flamefire Dec 1, 2024

ottml Dec 8, 2024

Flamefire Dec 1, 2024

Flamefire Dec 1, 2024

ottml Dec 11, 2024

Flamefire Dec 1, 2024

ottml Dec 8, 2024

Flamefire Dec 1, 2024

ottml Dec 14, 2024

Flamefire Dec 1, 2024

Flamefire Dec 1, 2024

ottml Dec 8, 2024

Flamefire Dec 1, 2024

ottml Dec 14, 2024

Flamefire Dec 1, 2024

ottml Dec 8, 2024

aztimh Dec 10, 2024

ottml Dec 11, 2024

aztimh Dec 11, 2024

ottml Dec 11, 2024

aztimh Dec 11, 2024

ottml Dec 11, 2024

aztimh Dec 11, 2024

ottml Dec 11, 2024

	if(job == Job::LeatherWorker)
	else if(job == Job::LeatherWorker)

Add leather addon #1720

Are you sure you want to change the base?

Add leather addon #1720

Conversation

ottml commented Dec 1, 2024

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment