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

fix: Fix transcription tooltip option not working correctly [skip ci] #2476

Merged
merged 3 commits into from
May 11, 2024

Conversation

ShadowCat117
Copy link
Contributor

Currently Wynnic/Gavellian text is always transcribed and coloured regardless of the tooltip config when it should only change the actual text if the tooltip config is disabled.

Discovery condition was removed as since the model is almost always out of date due to it taking significantly longer to query the content book it was basically never working.

@ShadowCat117 ShadowCat117 marked this pull request as ready for review May 11, 2024 14:25
@ShadowCat117 ShadowCat117 changed the title fix: Fix transcription tooltip option not working correctly fix: Fix transcription tooltip option not working correctly [skip ci] May 11, 2024
@kristofbolyai kristofbolyai merged commit 419aebe into Wynntils:main May 11, 2024
2 checks passed
@ShadowCat117 ShadowCat117 deleted the fix-transcribing branch May 12, 2024 17:16
@@ -351,21 +343,6 @@ private boolean hasTranscriber(WynnAlphabet transciberToFind) {
return false;
}

private boolean hasCompletedDiscovery(WynnAlphabet discoveryToCheck) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it is a bit sad to see that option go. I can say that I have never managed to complete any of these discoveries, and I've actually played with this setting on to kind of keep a bit of the suspense.

Can't we move this check to the character model instead, so we query the discoveries when the player enters a new world? Then it would be quick to check for this condition. I think we try to parse the quest book at that time anyhow, right?

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 think(?) we only scan the content book when the user opens the relevant screen, so discoveries would only be checked when the discoveries screen is opened. It could be done every world change but given the amount of discoveries it wouldn't be quick and I imagine we'd get complaints again about the inventory being replaced by the content book

Copy link
Member

Choose a reason for hiding this comment

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

I think that basically brings me back to my old idea of essentially caching the quest book locally, so we track any changes (and has a full-rescan fallback in case of issues). Having the quest book data instantaneuosly available without cost would probably open up a lot of cool stuff, such as automatically indicating when you are near a starting point for a quest that you have not done, etc.

Copy link
Member

Choose a reason for hiding this comment

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

So I agree, let's keep this removed for now, but if we do add the content book cache, we should re-implement this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to figure out a way to have time to do the query, we already do quite a lot on class join. But otherwise, great idea.

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

3 participants