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

Upgrade to pyside2 and construct 2.10 & working UI again #22

Merged
merged 15 commits into from
Feb 18, 2020
Merged

Upgrade to pyside2 and construct 2.10 & working UI again #22

merged 15 commits into from
Feb 18, 2020

Conversation

saibotk
Copy link
Contributor

@saibotk saibotk commented Jan 30, 2020

I took some time to upgrade the code to work with the new pyside2 and the newer construct version, while also using the newer python 3.8 to test it (i suppose it should also work well with python 3.7).
I also made sure, that the UI will work and compile properly again (some small issues are still there, as i first just wanted to upgrade the code).

Even though i tested most features (especially the .gma creation, which seems to be correct or rather provides the same results as before), this might need some more testing and a review.

One thing i could not yet fix is the parsed result for the addon_crc and MagicNumber, which is shown when using the --dump parameter. This seems to be an issue only with the parsing, as when i dump the file with the old binary of gmosh, it shows the correct addon_crc value and None for the MagicNumber, which i expected.
If anybody got some idea on how to fix this, i would be happy :)

I'd appreciate any feedback 👍

This was changed in December 2018 patch, see also:
handsomematt/glua-cache-extract@212c4e6
Only issue remaining: Seems like dumping a gma file data or parsing the data with the GMAVerifiedContents struct will return a wrong crc and a MagicNumber.
This is just an issue when parsing the data, as the data is still correct when using the old compiled version of gmosh...
This is due to the new SteamUGC addon format, so newer or recently updated addons cannot be downloaded via this method.
@FPtje
Copy link
Owner

FPtje commented Feb 2, 2020

Wow, you're throwing me a challenge here. I haven't really touched this project in ages, besides recently upgrading the gmpublish executable.

I've looked at it, and I've got a clue as to what's going wrong with the MagicNumber and addon_crc. I found that those two are at the very end of the GMA file. The definition of GMAVerifiedContents has a comment saying there's no terminator. That means the parser can parse only part of the GMA file and still succeed. So I temporarily changed the definition to this:

GMAVerifiedContents = 'GMAVerifiedContents'/Struct(
    GMAContents,
    'foo'/PaddedString(30, "utf8"),
    'addon_crc'/Optional(Int32ul),
    'MagicValue'/Optional(Int8ul)
    # Don't enforce terminator. Some GMA files appear to have 0-padding after the magic value
    # Terminator
)

And then dumped the GMA of Advanced Duplicator (pulled from my addons folder 😅). This is what it printed for the foo field:

foo = u'if (SERVER) then return end\r\n\r' (total 30)

Looks like the contents of some file! I haven't had the time to figure out why, but it looks like the file contents aren't being parsed here.

@FPtje
Copy link
Owner

FPtje commented Feb 2, 2020

Ok, leaving out the Lazy in 'all_file_contents'/Lazy(FileContents(Bytes(file_content_size))) gives the right result for Magic and addon_crc. There's probably something going on there. Probably something about embedding GMAContents in GMAVerifiedContents.

@Bo98
Copy link
Contributor

Bo98 commented Feb 3, 2020

I've not tested construct 2.10, but, as far as I can gather, Lazy no longer advances (seeks forward) the stream. I don't know if this is intentional or a bug - the whole thing was overhauled a bit.

A single-element LazyStruct or LazyArray might work as a workaround.

@saibotk
Copy link
Contributor Author

saibotk commented Feb 3, 2020

Sorry for giving you a challenge then 😅

I just thought it would be nice to at least make a PR with my changes, so it may be used by others and to give something back to this nice addon 👍, as i am regularly using it and would have loved to rebuild it / get to see the UI on Linux :)

So about the Lazy, i went through the upgrade guides and they removed the OnDemand function in construct 2.9. As i never used construct before, i just tried to get as close as possible to the original functions and Lazy seemed to be a good replacement, but as @Bo98 already said, the behavior differs significantly.

For my understanding:
Do we even need some kind of OnDemand/Lazy method for this? Or rather, what is the reason to have a lazy execution here?

Thanks for the feedback, for now i removed the Lazy, but i am open for suggestions on how to solve this in an more optimal way.

@FPtje
Copy link
Owner

FPtje commented Feb 3, 2020

Don't apologize for the challenge. This is a fun one!

The OnDemand was originally added to solve a significant performance problem. Quite a few operations on GMAs, like dumping, getting a file list, showing basic info, don't require parsing of the file contents. When working with big GMA files, like big maps, model packs, doing even basic things would take ages as the entire contents were loaded into RAM. With big enough files this can make the difference between tens of seconds and basically instantaneous operations.

@Bo98
Copy link
Contributor

Bo98 commented Feb 3, 2020

And it's arguably an even bigger problem now that GMod now supports GMAs up to 2GB.

@FPtje
Copy link
Owner

FPtje commented Feb 6, 2020

Using LazyStruct is indeed the solution. It seems as though Lazy doesn't jump properly when using a this field.

I've forked this PR in a branch called fix-ui-lazy. Commit 218c460 in there fixes the issue.

In gmoshui I'm getting an empty window when I open/extract a file:
sublime_text_2020-02-06_20-34-56

This empty window seems to be new.

@saibotk
Copy link
Contributor Author

saibotk commented Feb 6, 2020

@FPtje Awesome 👍
I rebased my branch, so your commit is applied.

Regarding the extra window, i did not specifically add it on purpose at least, this is the QDialog, that was previously also found in the createProgressDialog() function:
https://github.com/FPtje/gmosh/pull/22/files#diff-72f36ec21425f2a79028dd8a117ce39aL85-L87
But now that i look at it again, i guess this is now not needed anymore, i will adjust that asap.
👍

@saibotk
Copy link
Contributor Author

saibotk commented Feb 7, 2020

So i fixed that, also i found out that if you are searching a big lua cache (~6200 Files) the TreeView model cannot keep up that fast with applying the inefficient name filter for this many files. This will make the UI unresponsive, but it will still finish, it just might take long. So this is not related to the progress UI, just to let you know.

Is there anything else you want to be changed?

Thanks 👍

@FPtje
Copy link
Owner

FPtje commented Feb 7, 2020

Awesome! I don't think I do, but I'll check later.

@saibotk
Copy link
Contributor Author

saibotk commented Feb 8, 2020

Sadly i cannot seem to create a usable executable using cxfreeze for the gmoshui, even when using the old method of generating the python code using pyside2-uic from the .ui files. Maybe someone else does know more about the issue, as i am new to cxfreeze as well.
Side note: I could also push the changes for using pyside2-uic again and compiling the ui files, if you prefer that to the current method.

@FPtje
Copy link
Owner

FPtje commented Feb 8, 2020

Oh, you removed the pyside2 stuff. Yeah, you can put it back, we'll figure out a way to get it to work. I'll read into cxfreeze or maybe even something else if that works better.

@saibotk
Copy link
Contributor Author

saibotk commented Feb 8, 2020

Added it back in and it works, the only thing left would be getting cxfreeze to run again (only has issues with gmoshui) 👍

@Bo98
Copy link
Contributor

Bo98 commented Feb 8, 2020

I should see if cx_Freeze ever fixed the Python dependency issue on macOS. Looks like they've done several macOS-specific fixes over the past few months so they may well have.

@FPtje
Copy link
Owner

FPtje commented Feb 17, 2020

I just tested with cx_freeze and came across the following issue (on gmosh.py):
marcelotduarte/cx_Freeze#560

After trying the suggested fix (installing the master revision of cxfreeze), both gmosh and gmoshui created executables that ran. I ran the following commands:

pip install -U git+https://github.com/anthony-tuininga/cx_Freeze.git@master
mkdir bin > NUL
mkdir package\Windows\bin > NUL
mkdir package\Windows\required > NUL

pyside2-uic.exe ui/mainwindow.ui -o src/view/mainwindow.py
pyside2-uic.exe ui/progressdialog.ui -o src/view/progressdialog.py
cxfreeze src/gmosh.py --target-dir=bin --include-modules=addoninfo,gmafile,gmpublish,workshoputils
cxfreeze src/gmoshui.py --target-dir=bin --base=Win32GUI --include-modules=addoninfo,gmafile,gmpublish,workshoputils,atexit --icon res/icon.ico

What problems did you hit with cx_freeze?

@saibotk
Copy link
Contributor Author

saibotk commented Feb 17, 2020

Well i did not try the running the cx_freeze version from master, but that are great news 👍

EDIT: Nice, with the master version of cx_freeze, it builds correctly and works, awesome!

@FPtje FPtje merged commit b9e7195 into FPtje:master Feb 18, 2020
@saibotk saibotk deleted the fix-ui branch February 18, 2020 18:50
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