HoraceAndTheSpider / Amiberry-XML-Builder

Python script to scan LHA files for WHDLoad integration in Amiberry
13 stars 3 forks source link

[project update] To-Do #54

Closed nemo93 closed 4 years ago

nemo93 commented 4 years ago
  1. garbled line referenced in this ticket. I could "hardcode" the garbled characters to be removed from the script or the line could simply be removed... by hand. It's an important issue given it prevented some Retropie users from updating the file from within Amiberry.
  2. ~~I do have a very simple patch ready to implement "smart centering" in Amiberry. It's quite small, just a few lines to be added. I'm so glad to have the auto centering enabled eventually, it saves me a lot of time during testing!!! ~~ => submitted
  3. I'll keep submitting PR in that repo to add the smart centering and other small touches we were discussing.
  4. To discuss update of the xml bundled with Amiberry (the current one is 8 months old).
  5. To discuss the KS 2.05 aka A600HD.
HoraceAndTheSpider commented 4 years ago
  1. Hardcode the solution for now to skip the CUSTOM check for that particular variant of Benefactor only (CD32 slave v1.2). I've had tonnes of issues with my Pi-based joystick adaptor with random unsupported characters, and they always cause a pain on Python 3.0

I've checked the original slave and the fault is there as well, so i'll also report it on the Mantis bugtracker

Screenshot 2020-05-06 at 09 34 53
  1. No need to wait for my blessing on the main AMiberry PR, if you're happy with it, and @midwan is , then so am I :) Hopefully you found the other Amiberry XML code easy enough to read through.

Last night a 'full XML refresh' ran, which removed all of the Y-offsets. So the sooner we can get that sorted the better i think!

HoraceAndTheSpider commented 4 years ago

A further thought.

Is Amiberry going to default the X/Y Centering to "ON" ? If so, should the XML scanner not be looking fo games that wont use the centring option... ie.. Screen_NoCenter_H.txt ?

Edit: i should have scrolled back further on the Amiberry thread. So is the content of the files Screen_Center_H.txt those games where the function is OFF? If so we should rename the files to follow the existing convention (e.g. CPU_NoJIT.txt and Chipset_NoFastCopper.txt)

HoraceAndTheSpider commented 4 years ago

Also can you check Cybernoid 1 and 2 ingame..

These games have a copper bar that changes size at the top. I remember with PSPUAE this use to play havoc with the auto-screen sizing option!

nemo93 commented 4 years ago

Lots of good points. I'm reviewing all these right now :-)

  1. To me the default would be always set to "OFF". I'll rework the code for a simple if...else. Edit: I saw your comment and more checks to be added. I'm holding still.
  2. Fully agree also on the naming convention, my bad. I'll ensure to rename the files to Screen_NoCenter_H.txt and Screen_NoCenter_V.txt. So that whenever a subpath is added to either or both the centering value will be set to "none". => completed
  3. I'll check those Cybernoid games.
nemo93 commented 4 years ago

I'm starting to commit the changes on my branch about the script itself. I won't raise a PR now, it's late :-) I'll keep that for tomorrow.

I just wanted to raise the fact that I'm relying on lxml package mostly for sorting the xml. I had the intent to use more extensively but that's for a later stage. I'll add from lxml import etree into the script but wanted to be sure it's installed on the host were you run the script! If that's an issue let me know.

HoraceAndTheSpider commented 4 years ago

ah, i kind of wish you had submitted the PR, because i am sort of looking at it myself!! (Then i had to go to the shop)

Almost identical to your code except.... i couldnt work out if we need to specify SMART on.... maybe we do, but we should be careful on the Amiberry side to ensure that users have the option of having priority over the XML. This is why an 'order of precedence' can be quite important!

HoraceAndTheSpider commented 4 years ago

what is lxml needed to achieve exactly?

nemo93 commented 4 years ago

I could submit the PR now (you could check under my branch for the changes I've made) or I could delete those 2 small additions as I don't want to step on your toes!

lxml is needed to sort the many elements of the xml file in alphabetical order. If the sort is not required then let's forget about it!

HoraceAndTheSpider commented 4 years ago

Sort order shouldnt matter so lets park that for now (it's hard enough remembering to install lhafile everytime!) but please do submit the PR - i have already had a look ;)

nemo93 commented 4 years ago

PR submitted. Understood re lmxl, parked for now. Fully agree regarding the order of precedence and clearly I'd need help on that one as I don't want to mess anything up!

HoraceAndTheSpider commented 4 years ago

Item 1 of 3 in the OP is fixed :)

nemo93 commented 4 years ago

Thanks a lot @HoraceAndTheSpider! I feel ashamed as I let you down yesterday. You had to hardcode the "garbled chars" value yourself. Not a good day for me yesterday and I don't have as much time as I had in the past few weeks. I'm less responsive :-(

  1. I just issued a PR to remove blank lines and to delete also any line with non-ASCII char(s). I don't see valid reason to have non-ASCII char in the XML. Please keep me honest!

  2. in my repo I have a more robust version of the script where I do check if the XML is properly well-formed.

  3. the validation done in #2 is in fact achieved through... lxml \o/ And that's in fact the main reason I went with it in the first place. My response to you in the posts above was therefore partial. I don't like partial answer so please apologize. In short the script will throw a msg and write to a log file the precise error, eg. whdload_db.xml:26959:10:FATAL:PARSER:ERR_INVALID_CHAR: PCDATA invalid Char value Don't worry I'm not insisting at all. Just sharing reasons and answering precisely to your question (why lxml?). You have authority here!

  4. I'm afraid I won't have much time today and in the coming days. I'll see if I could sort the "hardware" parameters (l do like sorting stuff :-)) otherwise I'll keep on testing games and amending the "settings" files. Also I'll start looking at the mantis tracker as I'd like to submit requests as well for some games (Skidmarks is one for sure, Battle Isle is another etc.)

  5. I don't want to interject in the discussion you have with @midwan on the order of precedence/hostpref/amiberry.conf etc. As far as my tests went the .uae files always as precedence over the xml. Which is good and what I expect as user. => My (very small) 2cents: should users decide to rely on the XML then the best experience possible has to be provided. If users decide to go with .uae instead then they have full control and XML won't interfere here. I don't rely on hostspref or amiberry.conf hence not sure how they fit into that picture. => the small PR I submitted to Amiberry is working for sure (and it's a big time saver for my tests!!!) yet I'm really not sure of the changes to add in order to comply with the order of precedence you discuss with @midwan.

Edit: also thank you so much for adding me to the credits. I sincerely appreciate even I still consider myself more as a liability than anything else! Thanks again.

HoraceAndTheSpider commented 4 years ago

Firstly, never apologise for time constraints etc, and you certinaly didnt let anyone down. People need to accept we do this for love/fun/free but it all comes out of our own time.

  1. I added a comment on this -i understand that you commented out my 'quick fix' because non-ascii characters are now removed (which is great btw) but i think some of the faulty/messy items would still be valid characters, so I liked the fact this was a 'clean break', but I havent yet seen the final end result using your method, so i could be wrong here!!
HoraceAndTheSpider commented 4 years ago
  1. I understand.

  2. I will try a test script on the host machine for lxml - i do like the idea (and i am terrible at error checking) and I am just a little nervous about dependencies!

  3. No problem - any contribution is welcome, but none of it is obligatory. I'm grateful for everything you've done to keep this project on-track!!

  4. I think we have decided that hostprefs isnt needed any more. I am therefore probably happy that priority is; xml -> amiberry.conf -> uae ... but i will check with him and it may not even affect your submission at this stage

Lastly, like above, no thns required - you've put work in, so you deserve the credit!

nemo93 commented 4 years ago

Thanks again for your words and patience! I'm answering below about the PR.

My logic was a bit all or nothing re the non-ASCII chars. I decided to go with the easiest path which is: if any non-ascii character found on a line then delete that line. Therefore no further need to hardcode anything hence me commenting your quick fix (sorry about that. I was hesitating to do so hence the separate commit! - I could revert that no problem).

According to lxml running through your xml there was no other line with "invalid character". Therefore only the garbled line from benefactor cd32 has been removed. We should be good from now on yet there's no better thing than testing :-)

I could definitely think of a smarter way of doing this. Instead of squashing the line perhaps replace the invalid char by ' ' (space) or remove just that character for instance. => The scenario I'm most concerned about is where a filename would contain such invalid character for whatever reason.

  1. I forgot from my own to-do... I have perhaps one last change I'd like to push to your script. It's about the FASTRAM memory setting. As it is today in your script you won't ever go above 8MB of Fastram (and never parse the newly added Memory_FastRam_16.txt. Yet I found 2 occurences where more than 8MB of fast is required => goblins3 and the hdf version of Napalm crisis. The former is due to a bug apparently hard to fix and the latter is recommending 16MB as minimum (from the manual) and 32MB as recommended value! Yet Napalm crisis if I recall is not part of the xml leaving Goblins3 as the only candidate. Yet I'd like to extend to at least 16MB for Fast (note: Amiberry slider in the GUI won't go above 8MB though).
HoraceAndTheSpider commented 4 years ago

Right - i didnt take in that you were deleting the whole line. That is, actually, the better thing to do given this particular case, as this avoids the other 'still ascii' junk being there!

You shouldnt really need Fastmem >8mb ... if you need to specify more ram, then you can use Z3 ram for this. I already do this on a few other games.

Previous versions of UAE couldnt handle more than 8mb 'real fast' ram, which is why i have stuck with this, and the slider in Amiberry as you point out suggests the same, so why can Z3 ram not be used for Goblins and and Napalm?

nemo93 commented 4 years ago

My knowledge regarding the h/w spec is still quite limited. I was just a happy owner of a wonderful A500 back in the days :-)

In the case of goblins3 I really tried lots and lots of combination and the only one which works was to set FASTRAM to 16MB. To be honest the outcome is not even perfect as screen is flickering a lot to the point it's barely playable. But at least no more dreaded Err -6.

In fact this allows me to ask you a question I wanted to ask for long time :-) You may recall I've reported several games with "screen flickering" (Banshee, Akira, Roadkill). You always offered to add/increase the Z3 memory as a fix. Yet it never had any impact at least on my setup. However adding/increasing the Fastram solves this (usually to 8MB). What could be the explanation? I'm on a Pi3B+ if that makes any difference.

I've tried several combinations of memory settings for Napalm as well but same outcome. Game throws an error message where it states there's "not enough memory". I'll try with a uae where I'll set 32MB of Fast as per the manual. This one may be specific as I'm relying on a HDF version of a CD game! lots of unknown gems on Amiga CD btw... => I could create separate tickets to discuss those 2 games in particular.

Conclusion: I really need to better document myself re the difference between Fast/Z2 and Z3 for instance :) And how they work together...

HoraceAndTheSpider commented 4 years ago

There shouldnt be any visible difference between using fastram and Z3 that i know of, other than the cpu requirement is a bit different, which i tried to counter for in the amiberry setup code.

I will do some checks though when i have updated Amiberry on one of my Pi's!.