Skip to content
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.

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

XEH - Add missing event handlers with SPE 1.0 #1587

Closed
wants to merge 3 commits into from

Conversation

Dahlgren
Copy link
Contributor

When merged this pull request will:

  • Fix XEH issues with Spearhead 1944

class Tank;
class SPE_Armored_Target_Dummy: Tank {
class EventHandlers {
// This class has an empty CBA_Extended_EventHandlers, repopulate with actual event handlers
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, this is the SPE class structure

class SPE_Armored_Target_Dummy: Tank
{
	class Eventhandlers
	{
		class CBA_Extended_EventHandlers{};
	};
};

Since the XEH_ENABLED macro adds the EventHandlers classes with inheritance from the CBA_Extended_EventHandlers_base class it causes a load order issue and UBC problem. This structure with macros usage avoids both problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL. To fix, should put

class SPE_Armored_Target_Dummy: Tank {
	class Eventhandlers {
		delete CBA_Extended_EventHandlers;
	};
};

here, and then

    class Tank;
    class SPE_Armored_Target_Dummy: Tank {
        XEH_ENABLED;
    };

into ee component.

Copy link
Contributor Author

@Dahlgren Dahlgren Jul 25, 2023

Choose a reason for hiding this comment

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

That would need a separate (sub)config with cfgPatches requiredAddons[] = { "Some_SPE_Addon" }; and either the new skipWhenMissingDependencies with 2.14 patch or #if __has_include("some_spe_path")

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? delete complains now if no class is there? Maybe add it in patch 1, then remove it in patch 2, then XEH_enabled in patch 3. XEH already has a ton of (now sub-) module duplicates for legacy reasons. If you need to change the requiredAddons for it, go ahead.

Your current solution is whack.

Copy link
Contributor Author

@Dahlgren Dahlgren Jul 26, 2023

Choose a reason for hiding this comment

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

Why?

SPE is optional CDLC so not always loaded, when I tried locally CBA always got loaded before SPE so we need to ensure the load order unless we use the suggested solution in the PR

If you need to change the requiredAddons for it, go ahead.

Any preference on new skipWhenMissingDependencies which won't be usable until upcoming patch 2.14 and would block PR / release until then or using #if __has_include("some_spe_path") which doesn't work properly with some tools like HEMTT and binarization enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think #1589 should help with this problem

Copy link
Contributor

Choose a reason for hiding this comment

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

	class SPE_Armored_Target_Dummy: Tank
	{
		scope=1;
		class Eventhandlers
		{
			class CBA_Extended_EventHandlers
			{
			};
		};

it's scope=1 and nothing inherits from this

I'm guessing this is for something scripted,
they explicitly disabled eh and xeh on it
I think we might just want to trust them and not touch it

Comment on lines +23 to +29
class EventHandlers {
// This class has an empty CBA_Extended_EventHandlers, repopulate with actual event handlers
class CBA_Extended_EventHandlers {
EXTENDED_EVENTHANDLERS
};
};
SLX_XEH_DISABLED = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class EventHandlers {
// This class has an empty CBA_Extended_EventHandlers, repopulate with actual event handlers
class CBA_Extended_EventHandlers {
EXTENDED_EVENTHANDLERS
};
};
SLX_XEH_DISABLED = 0;
SLX_XEH_DISABLED = 1;

@PabstMirror PabstMirror added this to the 3.16.0 milestone Sep 6, 2023
@Dahlgren
Copy link
Contributor Author

Dahlgren commented Sep 6, 2023

This is likely to fixed in a future SPE update, should we skip this?

@jonpas
Copy link
Member

jonpas commented Sep 6, 2023

If it will be fixed, then yes. Can always merge after SPE updates if it ends up not being fixed.

@jonpas jonpas modified the milestones: 3.16.0, Backlog Sep 6, 2023
@jonpas
Copy link
Member

jonpas commented Dec 7, 2023

Has this been fixed in SPE 1.02 released on September 21st?

@jonpas
Copy link
Member

jonpas commented Mar 28, 2024

@Dahlgren?

@Dahlgren
Copy link
Contributor Author

Latest patch fixed some but these are still broken,

  • SPE_MINE_TMI42_Field_30x30
  • SPE_MINE_TMI42_Field_70x30
  • SPE_MINE_US_M1A1_ATMINE_Field_30x30

Config tree might be different than when this patch was maybe, it needs some love

@jonpas jonpas modified the milestones: Backlog, 3.18.0 Apr 24, 2024
@kerckasha
Copy link
Contributor

kerckasha commented Apr 24, 2024

I fixed the minefields eventhandler inheritance on SPE side, should be fixed next time an update comes out

@jonpas
Copy link
Member

jonpas commented Jun 13, 2024

If those minefields were the last, I think we can close this @Drofseh ?

@PabstMirror PabstMirror modified the milestones: 3.18.0, Backlog Jul 28, 2024
@Dahlgren
Copy link
Contributor Author

XEH issues fixed with SPE 1.1 patches

@Dahlgren Dahlgren closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants