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

Chat Updater Rewrite + GUI frontend, QOL changes #442

Merged
merged 39 commits into from Dec 11, 2022

Conversation

ScrubN
Copy link
Collaborator

@ScrubN ScrubN commented Nov 21, 2022

Changes, Fixes, and Additions:

CLI

  • Rewrite PreParseArgs.ConvertFromOldSyntax() to be more readable and flexible
  • Fix StatusInfo reports not fully overwriting the previous status
  • Bump some nuget package versions
  • Other cleanup

Core

  • Sealed runmode classes - This improves performance slightly by disabling inheritance
  • Move chatupdate functions from CLI to Core
  • Fix all embeds being doubled in chatupdate with arg --embed-missing
  • Fix crash in chatupdate when embeddedData property is null
  • Made chatupdate task oriented
  • Add the ability to move chat crops. Existing comments are preserved, and comments that were cropped out are fetched if possible
  • Add the ability to convert chat jsons to other formats
  • Made comment flooring math less complex by flipping operations (x * 1/y = x/y)
  • Add ability to disable retrieving comments and embeddedData properties from chat jsons to reduce useless memory usage
  • Dispose of embed tasks in ChatRender after copying to embed lists to further reduce memory usage
  • Redirect all mode cache folders to the root parent so they can be reused between modes
  • Change ReportType.Message[Info] to ReportType.Status[Info] for better clarity
  • Remove timestamp bool from ChatDownload as timestamp-format = None has the same functionality
  • Fixes Segmentation Fault on Render #448
  • Move json, html, txt chat operations to dedicated classes
  • Fix sample text height being computed every render tick as it is expensive
  • Rewrite SubstringToTextWidth() to use spans
  • Swap out some lists for arrays, swap out some strings for string bulders
  • Use foreach instead of LINQ to do emoji check - slightly faster
  • Combine some rtl and non-rtl functions
  • Only use DrawShapedText() with messages containing RTL characters
  • Minor TwitchHelper optimizations
  • Bump some nuget package versions
  • Other cleanup

WPF

  • Add GUI frontend for chatupdate
  • Made UI more consistent and readable
  • Add additional app icon scales - see comments for details
  • Bump some nuget package versions
  • Other cleanup

Other

  • Shorten chatupdate file/namespace/class names
  • Add Building from source section to regular README
  • Add ChatUpdate to CLI README
  • Update READMEs to be more accurate and fix typos

Notes:

  • This is a pretty big PR. Sorry for stealing your evening (:

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 22, 2022

I just learned something. With CommandLineParser, nullable bools cannot be assigned when the short char is used. I opted to make embed-missing default false and rather than removing its short char

@goldbattle
Copy link
Collaborator

I think it would be fine to remove the short char if we need to.

I have not tested this PR, but it is very promising (a much better implementation of trying to update chat information, instead of redownloading).

The problem with updating the comments/ crop is that the vod still needs to be up. I think you are checking for that already. The emotes and badges shouldn't require this check since it only depends on what has already been downloaded.

@goldbattle
Copy link
Collaborator

Could you explain where this bug was?

Fix all embeds being doubled with arg --embed-missing

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 23, 2022

Could you explain where this bug was?

Fix all embeds being doubled with arg --embed-missing

Using --embed-missing as a guard conditional was pretty smart, however I think it somewhat confused you when writing to chatRoot.embeddedData. Using firstparty as an example, you can see the embeddedData.firstParty is set to a blank list if it is null or --replace-embeds was passed, which is good. However in the case --embed-missing was passed, the list is left as-is. From there, the script enumerates through every emote from the response list and appends it to chatroot.embeddedData.firstParty, assuming the emote was not already in the list.

List<TwitchEmote> firstPartyEmoteList = await TwitchHelper.GetEmotes(chatRoot.comments, _updateOptions.TempFolder, chatRoot.embeddedData);

...

if (chatRoot.embeddedData.firstParty == null || updateOptions.ReplaceEmbeds)
{
    chatRoot.embeddedData.firstParty = new List<EmbedEmoteData>();
}
foreach (TwitchEmote emote in firstPartyEmoteList)
{
    EmbedEmoteData newEmote = new EmbedEmoteData();
    newEmote.id = emote.Id;
    newEmote.imageScale = emote.ImageScale;
    newEmote.data = emote.ImageData;
    newEmote.width = emote.Width / emote.ImageScale;
    newEmote.height = emote.Height / emote.ImageScale;
    chatRoot.embeddedData.firstParty.Add(newEmote);
}

The easy solution to this is to check the the ids of all the emotes already in the list against the id of the emote about to be appended.

...
foreach (TwitchEmote emote in firstPartyEmoteList)
{
    if (chatRoot.embeddedData.firstParty.Any(x => x.id.Equals(emote.Id)))
        continue;

    EmbedEmoteData newEmote = new EmbedEmoteData();
...

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 23, 2022

I think it would be fine to remove the short char if we need to.

Given that all of the other update options have short chars, I would rather leave the short char and make it default to false for the consistency

The problem with updating the comments/ crop is that the vod still needs to be up. I think you are checking for that already.

Yep, and I already have checks for that. I swapped out some throw new Exception() for throw new NullReferenceException in ChatDownloader because they are all thrown as a result of a null response from twitch and thus making them NRE is more descriptive. I also added an NRE check on the video id before anything is done so we don't need to wait for the response to throw for null or blank video ids.

Anyways doing this allows me to catch for only NREs which also means in the same block I can safely assume chatRoot.video.id (to be added by PR440) is not null. In the case that it IS null the same NRE catch block activates, which would have later been thrown by ChatDownloader anyways.

try
{
    // Only download missing comments if new end crop is greater than old end crop
    if (_updateOptions.CropEndingTime > chatRoot.video.end)
    {
        ChatDownloadOptions downloadOptions = GetCropDownloadOptions(/*chatRoot.video.id,*/null, tempFile, _updateOptions.FileFormat, chatRoot.video.end, _updateOptions.CropEndingTime);
        await AppendCommentSection(downloadOptions, tempFile, cancellationToken);
    }
}
catch (NullReferenceException)
{
    progress.Report(new ProgressReport() { reportType = ReportType.Log, data = "Unable to fetch possible missing comments: source VOD is expired or embedded ID is corrupt" });
}

Though you will see I have chatRoot.video.id commented out and am always passing in null because PR440 is still waiting to be merged. Again, once that is merged I will uncomment the chatRoot.video.id and remove the null. Similar situations appear in 3 other places in the file

@ScrubN ScrubN changed the title Chat Updater Update Chat Updater Rewrite + GUI frontend, QOL changes Nov 24, 2022
@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 24, 2022

I love when git inflates my line count and blames me by counting replacing spaces with tabs

@goldbattle
Copy link
Collaborator

Using --embed-missing as a guard conditional was pretty smart, however I think it somewhat confused you when writing to chatRoot.embeddedData. Using firstparty as an example, you can see the embeddedData.firstParty is set to a blank list if it is null or --replace-embeds was passed, which is good. However in the case --embed-missing was passed, the list is left as-is. From there, the script enumerates through every emote from the response list and appends it to chatroot.embeddedData.firstParty, assuming the emote was not already in the list.

Oh I thought that the functions would internally handle the duplicates by checking if they exists in the passed emote vector:

List<string> alreadyAdded = new List<string>();

The important functionality, is that the old emotes should NOT be replaced with newer version, just ones that are missing should be added (example is holiday emotes).

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 25, 2022

Oh I thought that the functions would internally handle the duplicates by checking if they exists in the passed emote vector:

Edit: I misread the first time, though I'll leave it anyways. alreadyAdded is only added to when the corresponding emote is also added to returnList, which is also partially why the doubling happens.

try
{
TwitchEmote newEmote = new TwitchEmote(emoteData.data, EmoteProvider.ThirdParty, emoteData.imageScale, emoteData.id, emoteData.name);
returnList.Add(newEmote);
alreadyAdded.Add(emoteData.name);
}

No, a standard list allows for duplicate entries. You could get around this by using a SortedSet<> and overriding the Comparer functionality to act predictably for your object type but by then you're overcomplicating the solution and doing an 0N2 bubble sort every time you add an object.

By then we may as well use the magic of LINQ to compare values before we even instantiate the new emote object, which the SortedSet<> solution would not be able to do

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 25, 2022

I realize that may seem hypocritical given we use a SortedSet<> for handling comment additions, however in that case we are using it to sort the comments by their offsets. That's more what SortedSet<> is for rather than using it to only remove duplicate elements

@ScrubN
Copy link
Collaborator Author

ScrubN commented Dec 2, 2022

I added more sizes to the WPF icon. Here's some pictures showing the difference it makes:
original
Note: the frame around the 64x64 is not because the file is selected, Windows adds that to improve readability ay high scaling factors.
new

Having the multiple resolutions is pretty important. Having just the low res obviously means no upscaling and having just the high res leads to really ugly nearest neighbor downscales from Windows. To make them I used GIMP to downscale the uncropped 1024x1024 PNG using NoHalo interpolation, cubic was pretty fuzzy.
layers

Move json, html, txt chat operations to dedicated classes,
Fix sample text height being computed every render tick as it is expensive,
Rewrite SubstringToTextWidth() to use spans,
Cleanup
…t emoji check from LINQ to foreach (faster), use arrays instead of lists, stringbuilders instead of strings where possible, combine some Rtl and non-Rtl functions, cleanup
@ScrubN ScrubN marked this pull request as ready for review December 11, 2022 21:47
@ScrubN ScrubN mentioned this pull request Dec 11, 2022
1 task
@lay295 lay295 merged commit 2def0e9 into lay295:master Dec 11, 2022
@ScrubN ScrubN deleted the chatupdate-patch branch December 11, 2022 23:34
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.

Segmentation Fault on Render
3 participants