Skip to content

[GEN][ZH] Fix ThingTemplate copying to prevent multiplayer mismatch with mod maps and quickstart #1234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[GEN][ZH] Fix ThingTemplate copying to prevent multiplayer mismatch with mod maps and quickstart #1234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffd2159
[ZH] Fix ThingTemplate copies to prevent mismatch with mod map and qu…
xezon Jul 6, 2025
92e3e36
Use CONSTEXPR
xezon Jul 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions GeneralsMD/Code/GameEngine/Include/Common/SparseMatchFinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,14 @@
#undef SPARSEMATCH_DEBUG
#endif

typedef UnsignedInt SparseMatchFinderFlags;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Unless I'm missing something, I think it'd be better to get rid of this typedef.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the imgui style of declaring enums for flags.

The reason for this is that

SparseMatchFinderFlags flags = flagName1 | flagName2;

does not work if SparseMatchFinderFlags is an enum. And it is bad to use int for flag types, because it is more difficult to follow what this flag type is then.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work if you do it inside the enum itself (flags1 = flagName1 | flagName2), which may not be feasible for larger enums, but fair enough.

enum SparseMatchFinderFlags_ CPP_11(: SparseMatchFinderFlags)
{
SparseMatchFinderFlags_NoCopy = 1<<0,
};

//-------------------------------------------------------------------------------------------------
template<class MATCHABLE, class BITSET>
template<class MATCHABLE, class BITSET, SparseMatchFinderFlags FLAGS = 0>
class SparseMatchFinder
{
private:
Expand Down Expand Up @@ -185,9 +191,29 @@ class SparseMatchFinder
return result;
}

//-------------------------------------------------------------------------------------------------
public:


//-------------------------------------------------------------------------------------------------
SparseMatchFinder() {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to define the constructor?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we should define a destructor if we're following the rule of 3, but in this case it'd be empty and C++98 doesn't support the default keyword.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a compile error when I did not add it. So I added that.

SparseMatchFinder(const SparseMatchFinder& other)
{
*this = other;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the copy flag be checked here as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is checked in the assignment operator (which this calls).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a little awkward to have the copy constructor call the copy assignment operator and also potentially slightly inefficient if the data is copied. I'd propose this instead:

SparseMatchFinder(const SparseMatchFinder& other) : 
	m_bestMatches((FLAGS & SparseMatchFinderFlags_NoCopy) == 0 ? MatchMap() : other.m_bestMatches)
{}

Copy link
Author

@xezon xezon Jul 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this construction was more simple then I would agree, but I think in this case it is easier to just write the code once for simpler C++ code. std::map does not allocate anything on default construction and there should be no worry about inefficiency - if the function was even called.

}

//-------------------------------------------------------------------------------------------------
SparseMatchFinder& operator=(const SparseMatchFinder& other)
{
if CONSTEXPR ((FLAGS & SparseMatchFinderFlags_NoCopy) == 0)
{
if (this != &other)
{
m_bestMatches = other.m_bestMatches;
}
}
return *this;
}

//-------------------------------------------------------------------------------------------------
void clear()
{
Expand Down
5 changes: 5 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/Common/ThingTemplate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,11 @@ class ThingTemplate : public Overridable
//Code renderer handles these states now.
//AsciiString m_inventoryImage[ INV_IMAGE_NUM_IMAGES ]; ///< portrait inventory pictures

// TheSuperHackers @bugfix Caball009/xezon 06/07/2025 No longer copy SparseMatchFinder to prevent copied instances linking to unrelated data.
// This avoids mismatching in certain maps, for example those that spawn units with veterancy.
typedef SparseMatchFinder<WeaponTemplateSet, WeaponSetFlags, SparseMatchFinderFlags_NoCopy> WeaponTemplateSetFinder;
typedef SparseMatchFinder<ArmorTemplateSet, ArmorSetFlags, SparseMatchFinderFlags_NoCopy> ArmorTemplateSetFinder;

// ---- STL-sized things
std::vector<ProductionPrerequisite> m_prereqInfo; ///< the unit Prereqs for this tech
std::vector<AsciiString> m_buildVariations; /**< if we build a unit of this type via script or ui, randomly choose one
Expand Down
3 changes: 0 additions & 3 deletions GeneralsMD/Code/GameEngine/Include/GameLogic/ArmorSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,4 @@ class ArmorTemplateSet
//-------------------------------------------------------------------------------------------------
typedef std::vector<ArmorTemplateSet> ArmorTemplateSetVector;

//-------------------------------------------------------------------------------------------------
typedef SparseMatchFinder<ArmorTemplateSet, ArmorSetFlags> ArmorTemplateSetFinder;

#endif // _ArmorSet_H_
3 changes: 0 additions & 3 deletions GeneralsMD/Code/GameEngine/Include/GameLogic/WeaponSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,6 @@ class WeaponTemplateSet
//-------------------------------------------------------------------------------------------------
typedef std::vector<WeaponTemplateSet> WeaponTemplateSetVector;

//-------------------------------------------------------------------------------------------------
typedef SparseMatchFinder<WeaponTemplateSet, WeaponSetFlags> WeaponTemplateSetFinder;

//-------------------------------------------------------------------------------------------------
enum WeaponChoiceCriteria CPP_11(: Int)
{
Expand Down
Loading