Skip to content

Configurable resolution#198

Open
tasuki wants to merge 6 commits intosuomipelit:masterfrom
tasuki:configurable-resolution
Open

Configurable resolution#198
tasuki wants to merge 6 commits intosuomipelit:masterfrom
tasuki:configurable-resolution

Conversation

@tasuki
Copy link
Copy Markdown

@tasuki tasuki commented Mar 3, 2025

Fixes #195

This is still a work in progress ⚠️ 🚧 but I'm looking for your comments!

Copy link
Copy Markdown
Collaborator

@akx akx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick initial comment before I look through all of this:

Based on

$ ag -o "scr_._size\s*/\s*\d+" | grep : | cut -f3 -d: | sort | uniq -c | sort -nr
  45 scr_x_size/2
  32 scr_x_size / 2
  12 scr_y_size / 20
  10 scr_y_size / 2
   8 scr_x_size / 20
   5 scr_x_size/4

it would probably make sense to at least introduce the variables scr_x_half_size and scr_y_half_size, so as to

  • make the intent clearer where these are being used
  • avoid two different whitespace "spellings" of the division
  • (and to avoid the extra runtime divisions, not that that matters on modern hardware)

@tasuki
Copy link
Copy Markdown
Author

tasuki commented Apr 17, 2025

I actually used to have a scr_x_half_size variable and decided to delete it because in the end I found it less readable. (I also added a scr_x_quarter_size and that made things even worse. And then there's the dividing by 20. There are so many magic numbers!)

The right solution would imo be to build up more powerful abstractions: a lot of the DRAW.CPP file is just copy-paste of the same logic, with minor variations. I don't feel competent to do that, though I guess soon enough a LLM will be able to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Configurable resolution

2 participants