search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Map::MapTileWidth() and Map::MapTileHeight() are ambiguous #271

Closed ldicker83 closed 5 years ago

ldicker83 commented 5 years ago

Started using OP2Utility, specifically for loading maps. The functions MapTileWidth() and MapTileHeight() are named such that they would give the tile dimensions versus the map dimensions.

These should either be MapWidth()/MapHeight() or MapTileCountX()/MapTileCountY().

DanRStevens commented 5 years ago

A lot of internal calculations are based in pixels, while others are done in tiles. Hence MapWidth/MapHeight would be ambiguous.

I've never much enjoyed the different lengths of the two names though, so maybe an "X"/"Y" difference would be better?

ldicker83 commented 5 years ago

Urgh, the devs made a lot of really weird decisions during development. I'll leave it up to your judgement, just wanted to note that it surprised me when it didn't have the values I expected.

I did also note a distinct lack of comment documentation for the various functions. From tearing the code apart before in OP2Landlord I was at least familiar with how the maps deal with it but to the unfamiliar the functions and structure of the map would be baffling. Comments for the functions would have cleared that up pretty much right away.

Anyway, I can add documentation if you'd like.

DanRStevens commented 5 years ago

Please do. And maybe discuss with Brett about method name changes. I noticed Brett also like the "...Count" style names. I'm guessing that comes from C# programming conventions?

Edit: Oh geeze, I only just now realized what your interpretation was, and where the confusion comes from.

ldicker83 commented 5 years ago

Yup. :)

I'll spend a little time getting the functions documented. Aside from some assumptions I made that were incorrect the code appears to work well.

Brett208 commented 5 years ago

I'd like to change the function name to be:

Map::WidthInTiles(); Map::HeightInTiles();

Any issues with that? I think it is better defined than the current names. Leeor, please feel free to add documentation. We are trying to keep from being overly verbose, I you are correct that we are lacking right now.

-Brett

ldicker83 commented 5 years ago

That resolves any ambiguity. Go for it.