huderlem / poryscript

High-level scripting language for gen 3 pokemon decompilation projects
https://www.huderlem.com/poryscript-playground/
MIT License
197 stars 22 forks source link

Add additional command line flags for default format options #42

Closed Jademalo closed 3 years ago

Jademalo commented 3 years ago

This adds two new flags;

  -f string
        set default font (leave empty to use first font in JSON)
  -l int
        set default length of line of formatted text (default 208)

-l allows you to change the default maximum text width. This simply replaces the hardcoded 208 value. -f allows you to specify a default font via the command line, rather than selecting it from within the JSON. In addition, I've modified it so that instead of using a value set in the JSON to denote the default, in the event of no font being specified with -f it uses the first font entry in the JSON.


I was originally going to leave this as a suggestion, but realised it was fairly easy to just implement myself.

The reason I implemented this was due to the formatting being incorrect for pokefirered. fr/lg has a space with a width of 6 pixels compared to r/s/e's 3 pixels. This resulted in the text width being incorrect most of the time. Initially I thought it was because fr/lg had a different width textbox, which is why I implemented -i. I later realised it was because the space width was different.

I figured that being able to set the default via the command line would be useful, and also having a second set of widths available natively for fr/lg would be a bonus. From there I figured the best way to approach the default if no flag was set was to just have it be the top entry in the json, since it eliminates a point of error.

The documentation should be updated, and the tests all pass. I had to modify the tests since the function had more args now, but as far as I'm aware those modifications shouldn't affect the functionality of those tests. One expected result had to be altered, since there are now two fonts in the list.

Jademalo commented 3 years ago

Hmm, those test fails are strange. On my machine the list is returned in the order it's defined in the json, so with 1_latin first and 1_latin_frlg second.

codecov-commenter commented 3 years ago

Codecov Report

Merging #42 (22a9d66) into master (6d013ff) will decrease coverage by 0.28%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   92.09%   91.81%   -0.29%     
==========================================
  Files           6        6              
  Lines        2050     2052       +2     
==========================================
- Hits         1888     1884       -4     
- Misses        102      105       +3     
- Partials       60       63       +3     
Impacted Files Coverage Δ
parser/parser.go 90.00% <100.00%> (-0.17%) :arrow_down:
parser/formattext.go 93.83% <0.00%> (-2.74%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6d013ff...22a9d66. Read the comment docs.

huderlem commented 3 years ago

JSON fields (and Go maps) have no order. That's why your tests are flaky, and it's also the reason Poryscript shouldn't default to the "first" font in the config file. I think the "defaultFontId" field in font_widths.json should stay. The -f flag can then simply override that, though it seems unnecessary because you can already control the default font using the JSON config file.

EDIT: To be more clear, I'm asking you to:

  1. Reinstate "defaultFontId" in the font_widths.json file, and remove any concept of "first" font in that file.
  2. Possibly remove the -f flag, unless you think that's still useful with defaultFontId existing at the same time.
Jademalo commented 3 years ago

Huh, that's weird, I would've expected them to be ordered. That makes sense though, I'll revert that. Under the assumption of JSON being ordered, I thought that would just be a simpler way of doing it.

With regards to the -f flag, I think it still has benefits for a few different situations, especially now that I've implemented a correct width map for fr/lg. Being able to specify -f 1_latin_frlg -l 198 in the makefile solves every issue for fr/lg, and makes setting up for those games as simple as for r/s/e. It would also allow for more complex future implementations where different fonts are needed for groups of maps without having to manually specify it in every textbox. I personally feel it's better to have this implemented as a flag, with the option in font_widths.json being more of a fallback.

I'll implement those changes now.