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

Add basic nameplate service #1830

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

Pilzinsel64
Copy link

@Pilzinsel64 Pilzinsel64 commented Jun 8, 2024

grafik

Keep in mind that this is just a basic (but futureorientate) implementation, re-implemented inspired by Pilz.Dalamud. The current implementation uses the SetPlayerNameplateDetourDelegate event and provides just a basic way to edit the existing elements of a player nameplate.

It's not possible yet to directly get/edit/add/remove nodes. This can be done in a future PR by a future implementation of the Nameplate service wich uses another Event (more low-level wich then access AktResNode etc.). But I had not the time to get involved with this Event. Most plugins uses this one and that's enough for now imao.

I know the other event is more low-level that gives you more options. But I have no expierience with it and time to investigate and it's not really required for this basic PR in my opinion.

Also, the manual allocation has it's reason. MemoryHelper.WriteString didn't work. The parameters like titlePtr are addresses to raw byte arrays, so Utf8String isn't working too.

@Pilzinsel64 Pilzinsel64 requested a review from a team as a code owner June 8, 2024 14:54
@Pilzinsel64 Pilzinsel64 marked this pull request as draft June 8, 2024 14:57
Dalamud/Game/Gui/Nameplates/Model/NameplateInfo.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/Model/NameplateObject.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/Model/StatusIcons.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/NameplateGui.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/NameplateGui.cs Outdated Show resolved Hide resolved
@MidoriKami
Copy link
Contributor

@Infiziert90 noticed that you are using IntPtr everywhere, should use nint instead.

@Pilzinsel64
Copy link
Author

@Infiziert90 noticed that you are using IntPtr everywhere, should use nint instead.

Changed everything to nint now.

@MidoriKami
Copy link
Contributor

Is there a particular reason or reasons why this is still a draft?

@Pilzinsel64
Copy link
Author

Pilzinsel64 commented Jun 15, 2024

@MidoriKami I didn't actually tested this via the Tesplugin yet as I had no time this week. :/
Aaand I didn't took a look to the Status Icon Excel Sheet you asked me for to check. 😅

But mainly just thought it's better to keep it as draft before I (or someone) actually tested it. xD

@MidoriKami
Copy link
Contributor

Understandable, not trying to rush, it just seemed like it was pretty close to being good to go, we can always make minor adjustments later.

@Pilzinsel64 Pilzinsel64 force-pushed the nameplate-api-basic branch 3 times, most recently from ce01bfa to 7f410c3 Compare June 18, 2024 03:13
@Pilzinsel64
Copy link
Author

Alright, ready for review/merge now!
grafik

Keep in mind that this is just a basic (but futureorientate) implementation, re-implemented inspired by Pilz.Dalamud.

It's not possible yet to directly get/edit/add/remove nodes. This can be done by a future implementation of the Nameplate service wich uses another Event (more low-level wich then access AktResNode etc.). But I had not the time to get involved with this Event. Most plugins uses this one and that's enough for now imao.

Also, the manual allocation has it's reason. MemoryHelper.WriteString didn't work. The parameters like titlePtr are addresses to raw byte arrays, so Utf8String isn't working too.

@Pilzinsel64 Pilzinsel64 marked this pull request as ready for review June 18, 2024 06:33
@kalilistic
Copy link
Contributor

Will this be merged in with apix? No pressure, but we're talking about falling back to the library if not and want to plan accordingly.

@goaaats
Copy link
Member

goaaats commented Jun 28, 2024

That would be nice, but it'd have to go in this weekend. Could you please re-review if you have time? @Haselnussbomber, @nebel and @MidoriKami - thank you!

Copy link
Contributor

@MidoriKami MidoriKami left a comment

Choose a reason for hiding this comment

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

Some concerns that I think need to be addressed immediately before it can be merged, and some suggestions that would be nice to have.

I won't be available for follow up reviews until I have completed MSQ.

Dalamud/Game/Gui/Nameplates/Model/NameplateElementName.cs Outdated Show resolved Hide resolved
Dalamud.CorePlugin/PluginImpl.cs Outdated Show resolved Hide resolved
/// Gets all available nameplate elements.
/// </summary>
/// <returns>Returns an array of all nameplate element.</returns>
INameplateElement[] GetAllElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a specific reason why this is an Array instead of a List?

Dalamud/Game/Gui/Nameplates/Model/INameplateObject.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/Model/NameplateElement.cs Outdated Show resolved Hide resolved
[ServiceManager.ServiceDependency]
private readonly AddonLifecycle addonLifecycle = Service<AddonLifecycle>.Get();

[Signature("E8 ?? ?? ?? ?? E9 ?? ?? ?? ?? 48 8B 5C 24 ?? 45 38 BE", DetourName = nameof(HandleSetPlayerNameplateDetour))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in a AddressResolver class, see AddonLifecycleAddressResolver, and AddonLifecycle for an example.

Dalamud/Game/Gui/Nameplates/NameplateGui.cs Outdated Show resolved Hide resolved
Dalamud/Game/Gui/Nameplates/NameplateGui.cs Outdated Show resolved Hide resolved
Comment on lines +195 to +221
private nint PluginAllocate(SeString seString)
{
return this.PluginAllocate(seString.Encode());
}

private nint PluginAllocate(byte[] bytes)
{
var pointer = Marshal.AllocHGlobal(bytes.Length + 1);
Marshal.Copy(bytes, 0, pointer, bytes.Length);
Marshal.WriteByte(pointer, bytes.Length, 0);
return pointer;
}

private void PluginFree(nint ptr)
{
Marshal.FreeHGlobal(ptr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there has to be a better way, but we can look into that later, as this is implementation details that we can address at any time without breaking the API.

Dalamud/Plugin/Services/INameplatesGui.cs Outdated Show resolved Hide resolved
@nebel
Copy link
Contributor

nebel commented Jun 28, 2024

That would be nice, but it'd have to go in this weekend. Could you please re-review if you have time? @Haselnussbomber, @nebel and @MidoriKami - thank you!

There is currently an issue with this service which could be quite difficult to address, namely that the main function we hook to do all the work (SetPlayerNamePlate) has been fully inlined in 7.0. The original function still exists but it is no longer called, and our hook does nothing. Until we can figure out how to fix this, I think we need to hold off on merging this.

@Pilzinsel64
Copy link
Author

Pilzinsel64 commented Jun 29, 2024

Arg, that was unintended... Maybe they moved it to the paren function now.

Hm, there is another function we could hook to. But I don't know how it's called (maybe something like NameplateUpdate) and not what the sig is. I know it's more low level and is called for every single namplate when updating the stringdata, so also NPCs, if I'm right. But I have absolutely no experience with it.

@goaaats goaaats deleted the branch goatcorp:master July 1, 2024 18:48
@goaaats goaaats closed this Jul 1, 2024
@KazWolfe KazWolfe reopened this Jul 1, 2024
@KazWolfe KazWolfe changed the base branch from apiX to master July 1, 2024 18:54
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.

None yet

7 participants