Skip to content

Conversation

@robojumper
Copy link
Member

@robojumper robojumper commented Feb 7, 2020

In which we work our way into the innards of the UnrealScript virtual machine. Closes #767.

Original description below, see comments for current design:

Click to expand

// Must be called on a `Class` instance, i.e. if you have an X2AbilityTargetStyle and
// want to know whether it's native, check `TargetStyle.class.IsNative()`
final function bool IsNative()
{
	// This is the flag the UC VM uses to mark a class as native
	return (self.ObjectFlags.B & (1 << 26)) != 0;
}

You may call <SomeObject>.class.IsNative() to determine whether the class is native, or, if you are working with a class<_> already, skip the .class part.

Because I am a skeptical person and you should be too, here is a debug output that makes this implementation appear correct (interestingly enough, GetClassDefaultObjects yields objects for native classes first):

https://pastebin.com/acG9Jc0a

This may come in useful for #762.

@Xymanek
Copy link
Member

Xymanek commented Feb 7, 2020

Why not check if (class(self) != none) (or vice versa) and adjust the check automatically, so that you could call the method directly, without caring about "type of instance" that you are dealing with?

@robojumper
Copy link
Member Author

I'm conflicted. On one hand, it's more "magic" and can clash with the mental model where objects and their classes are entirely separate things (see #477 (comment), where I got confused about this) and introduces an inconsistency with other functions in Object (see discussion I just linked).

On the other hand, the only kinds of Object that can have this flag set (and for which the function makes sense) are classes, but I can't add it to Class.uc because that doesn't exist.

Perhaps a

final static function bool ClassIsNative(Class klass)
{
	// This is the flag the UC VM uses to mark a class as native
	return (klass.ObjectFlags.B & (1 << 26)) != 0;
}

would be better?

@Xymanek
Copy link
Member

Xymanek commented Feb 7, 2020

Yeah, ClassIsNative resolves the confusion 👍

@MrNiceUK
Copy link
Contributor

MrNiceUK commented Feb 7, 2020

Assume that's "as well as", so you call IsNative() or IsClassNative() depending what you're dealing with?

@robojumper
Copy link
Member Author

No, a static function ClassIsNative.

Copy link
Member

@Xymanek Xymanek left a comment

Choose a reason for hiding this comment

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

Needs a tracking issue

@robojumper robojumper marked this pull request as ready for review February 8, 2020 09:00
@robojumper robojumper changed the title Add Object:IsNative to check whether a class is native Add Object:IsClassNative to check whether a class is native Feb 8, 2020
@robojumper robojumper requested a review from Xymanek February 11, 2020 13:35
@Xymanek
Copy link
Member

Xymanek commented Feb 11, 2020

What about #765?

Copy link
Member

@Xymanek Xymanek left a comment

Choose a reason for hiding this comment

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

Please adjust the version info/check, now that the new approach is implemented

@robojumper
Copy link
Member Author

Fixed. I hope.

Comment on lines -37 to -35
if (class'Engine.CHEngineVersion' != none)
{
class'X2TacticalGameRuleset'.static.ReleaseScriptLog("X2WOTCCommunityHighlander: Creating Engine version template...");
`CREATE_X2TEMPLATE(class'CHXComGameVersionTemplate', XComGameVersion, 'CHEngineVersion');
XComGameVersion.MajorVersion = class'Engine.CHEngineVersion'.default.MajorVersion;
XComGameVersion.MinorVersion = class'Engine.CHEngineVersion'.default.MinorVersion;
XComGameVersion.PatchVersion = class'Engine.CHEngineVersion'.default.PatchVersion;
XComGameVersion.Commit = class'Engine.CHEngineVersion'.default.Commit;
class'X2TacticalGameRuleset'.static.ReleaseScriptLog("X2WOTCCommunityHighlander: Created Engine version template with version" @ XComGameVersion.MajorVersion $ "." $ XComGameVersion.MinorVersion $ "." $ XComGameVersion.PatchVersion);
Templates.AddItem(XComGameVersion);
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this backwards incompatible? Since someone might be checking the engine version already

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes, but the only Engine feature we have is the AudioComponent WWise/SoundCue fix which I don't think anyone ever queried for.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think anyone ever

So far this kind of thought was allowed only in extreme cases, particularly related to new features

@Xymanek
Copy link
Member

Xymanek commented Mar 3, 2020

Also, the matter of name collision still remains. I think naming the function CH_IsClassNative would be much safer option


Also 2: Shouldn't /// HL-Docs: ref:ComponentVersions be added to the DLC2 version?

@robojumper
Copy link
Member Author

Addressed. I would still like to get rid of the Engine template because it's not useful and there is no mod that ever checked that Engine version (it only contained a single bugfix).

@Xymanek
Copy link
Member

Xymanek commented Mar 3, 2020

I mean, if you are so sure that no one ever checked the engine template, then I guess go ahead....

@robojumper robojumper merged commit 6dfae4c into X2CommunityCore:master Mar 3, 2020
@Musashi1584 Musashi1584 added this to the 1.21.0 milestone Apr 13, 2020
@robojumper robojumper deleted the nativity_check branch September 3, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Object:IsClassNative

4 participants