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

Switch INotificationManager and ITitleScreenMenu to use ISharedImmediateTexture #1879

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

Conversation

Critical-Impact
Copy link
Contributor

INotificationManager

  • Simplifies the API, no longer can you pass in a IDalamudWrap or Task, if a user wants to pass in a already created texture they can use ForwardingSharedImmediateTexture or implement their own implementation of ISharedImmediateTexture
  • There is no longer an option to leave textures open, they can RentAsync a ISharedImmediateTexture or create a texture and dispose of it as they see fit

ITitleScreenMenu

  • Users can now only pass in a ISharedImmediateTexture as per INotificationManager
  • The icon will only be validated on draw(that it's 64x64), if it is invalid, the entry will be removed from the list and a error will be logged.

As mentioned above this PR also adds in a wrapper class for IDalamudTextureWrap if the user creates the texture wrap themselves called ForwardingSharedImmediateTexture

@@ -317,11 +268,6 @@ internal void RemoveNonDalamudInvocations()
if (this.Icon is { } previousIcon && !IsOwnedByDalamud(previousIcon.GetType()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this line so that IsOwnedByDalamud check is also done for the IDTW stored inside ForwardingSharedImmediateTexture.

@goaaats
Copy link
Member

goaaats commented Jul 3, 2024

We only have two options here now, sadly:

  1. Add wrapper APIs that automatically convert texture wraps to ForwardingSharedImmediateTexture and tag them as Api11Obsolete
  2. Wait until 7.1 or whenever we can break it again.

Sorry, this is probably on me. We should have kicked this off a lot earlier. Breaking it is no longer an option now.

@Critical-Impact
Copy link
Contributor Author

Critical-Impact commented Jul 4, 2024

@goaaats
I've only given this a cursory test as I'll be busy for most of tonight but I've done what I can in terms of adding back the missing APIs and then having the redirect to the right place.

The primary caveat with this is that it awaits the result of the old Task based methods. This could mean potential hitching as it loads but without creating a level of indirection that I don't really want to do it's about the best I could come up with.

The ITextureProvider changes means that if they are actually providing a Task I'd be very inclined to say it's already a loaded wrap so I'd say the above point is moot but opinion may differ

}
else
{
var dalamudTextureWrap = value.Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Assigning failed tasks should not throw. IMO it's better to just leave previous properties/methods alone and add ISIT on its own, and only on drawing refer to ISIT at first and then check the rest to figure out the correct thing to draw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd basically need to throw out everything I've already done if that's the route you want to go, as it'd be faster for me to start on a new branch and implement a ImmediateIconTexture there rather than trying to put back in what I've already removed

Have added in a try/catch, not pretty but does aleviate that above concern

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully nobody refers to the state of the task after assigning it to the notification, too. Not sure why would one do that, so I'd guess it's be safe for this property to be "inconsistent".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only thing I could think of is creating a new Task.FromException<IDalamudTextureWrap?>(exception), storing it in a field and returning that when IconTextureTask.get is called

Then when a set resolves correctly, it sets the failed task field to null.

Not sure why would want to do it either, if I don't do something like above, it shouldn't cause a crash as they'll still have to null check, just a bit incosnsistent as you said

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possible way would be making ForwardingSIT store Task instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've considered it however it'd require the GetWrapOrEmpty function to call Service<DalamudAssetManager>.Get().Empty4X4 which adds a dependency on an internal service.

Now if we made it so you could only get a ForwardingSharedImmediateTexture's from the ITextureProvider and it injected the 4x4 into the ForwardingSharedImmediateTexture that'd be fine but it adds a hard dependency to an object that could concievably be constructed outside of Dalamud

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.

3 participants