Retera / WarsmashModEngine

An emulation engine to improve Warcraft III modding
GNU Affero General Public License v3.0
192 stars 37 forks source link

Redesign of map-selection UI #23

Closed bearU369 closed 1 year ago

bearU369 commented 1 year ago

Adjustments to look as similar to the original. Remain on draft until I finished doing the tinkering on below:

Screenshot: image

bearU369 commented 1 year ago

Have managed to implement the UI and counters in map list after hours of trying to figure out how to create and render Frames. Although I heavily modified ListBoxFrame without realizing that some parts of the code rely on, breaking them. I should implement a new Frame class and move the changes there 🙃. image

bearU369 commented 1 year ago

Think that's about it and it should be good enough for merge. Some issues to point out at advance:

Retera commented 1 year ago

This seems cool! Sorry that you found yourself dealing with the UIFrame system. As I imagine you can tell by now, this is a system that I mostly threw together from my own design to try to "inflate" the UI files in a way that would try to visually match their intention. Currently what this achieves is to use files of a particular format, rather than to make the most ideal system possible, surely.

In a future update, rather than to load the map information (player count, map name, melee map status) from the informational files within the map archive, for high performance the map format if I'm not mistaken contains a small binary header at the top of the file that is not stored within the MPQ archive and that can be loaded more quickly (no archive extraction) to get this same metadata fast for all maps in the folder. There is a website here that I believe was attempting to document the format years ago: https://world-editor-tutorials.thehelper.net/cat_usersubmit.php?view=42787

It may be the case that we will still need a backup system to fetch this information from within the archive like what is being done in this current iteration here in your PR, because I may have heard somewhere that in order to discard old systems intended to have higher performance and possibly reduce the performance, Reforged changed the map format so that possibly the Reforged World Editor is saving maps that no longer have the header for high performance reading. I have not verified that, however, and I may largely not care. I think that skimming the top 512 bytes off of each map in the folder in order to preview it sounds like it might be a nice system, so if someone else is trying to push out a map file format design that reduces performance and eliminates the means for doing that, we could simply tell them that we don't want to support it and push back.

bearU369 commented 1 year ago

Thanks. The UIFrame system is a bit complex to grasp and understand how are the frames being utilized so it took me a while to read through the source code. I started messing around with the UIFrames because I like seeing the UIs looking fancy. 😛

It would be great to able to update the frames directly from the map information, without storing them into variables. Though I don't really understand much how map and MPQ formats work so I rely with your methods that I don't know how to program with.

I have the extra variables in MapItem because that's where the item frames would update their texts to on the fly, as storing only the rawPath mean that it must extract and parse the map file before it would update the frames, every time the user is navigating through the list. The only time it will do the extraction and parsing is when it is being added as a item, to prevent lagging the game from the two aforementioned processes.

Retera commented 1 year ago

I put a review comment onto here a couple of days ago but I have more experience using BitBucket than GitHub to be honest and I didn't realize that my review had not published to where you could see it yet. It was for something pretty minor. Do you want to fix the minor suggestion or should I merge the PR and fix it on my side afterwards?

Partially I'm wondering if after merging this PR if the plan is to also merge the expandable ListBoxFrame PR that might be able to replace some of the work here? I haven't read through that one as much yet.

bearU369 commented 1 year ago

I'll patch the minor suggestion before the merge, don't want to open another PR for it.

Also if #24 got merged, I'll replace the MapListBoxFrame with ItemProperty/Display. This PR barely touches the actual ListBoxFrame so it should be an easy fix.