search

TeamJM / journeymap-legacy

GNU Lesser General Public License v2.1
10 stars 13 forks source link

Improve map grid size handling, fix fullscreen map cutting off #35

Closed leumasme closed 1 month ago

leumasme commented 2 months ago

First commit makes the grid size dynamic instead of a constant 5x5. Fixes #34

Other 2 commits clean things up and make the grid able to be non-square, e.g. 5 tiles in width and 3 tiles in height. This should reduce the amount of work done. when playing in fullscreen on a 1920x1080 screen, only 3x5=15 tiles are used instead of 5x5=25. I have not benchmarked the actual impact of this as I'm not sure how to do this here. These also create changes in public functions, which may count as a breaking (major version increase) change, depending on your versioning. If these other things are unwanted, I can drop the commits.

All changes are tested ingame. Proper review and testing would still be appreciated as i find this codebase a bit confusing.

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

leumasme commented 1 month ago

did you decompile that code to look?

I did take a look at the recalculation logic, but I think that was the only sensible way to implement this anyway. Looks like later releases still use a square tile size though(?); I wonder what the performance impact of that is. As a note, the formula for the required tile size was also slightly suboptimal in the version I looked at; ceil(windowsize / tilesize) + 1 may calculate a number that's larger than required (e.g. 4 for 1080p -> 5 tiles to make it odd, even though only 3 are required) ceil(windowsize / tilesize + 0.5) is more accurate to represent the possibility panning away from the center by up to half a tile, and correctly yields 3 for 1080p screen height, 512 tile size

mysticdrew commented 1 month ago

@leumasme see #36

leumasme commented 1 month ago

Looks like minimap calculation is coming up with needing only 1x1 tiles? Maybe just let it be minimum 3x3, think that should fix it (not 100% sure). Alternatively change formula to be ceil(cells)+1 instead of ceil(cells+0.5), the exact thing I wrote about before... But that may result in some unnecessary work done on specific resolutions. Sadly I'm not home for the next 4 days so I can't do it myself, sorry for the bug.

mysticdrew commented 1 month ago

I am also going to be very busy until next week. Users can use the old version until a fix can be completed.