Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible duplication of functionality in screenshot.cpp #8252

Open
techgeeknz opened this issue Jul 2, 2020 · 4 comments
Open

Possible duplication of functionality in screenshot.cpp #8252

techgeeknz opened this issue Jul 2, 2020 · 4 comments

Comments

@techgeeknz
Copy link
Contributor

@techgeeknz techgeeknz commented Jul 2, 2020

Version of OpenTTD

Since e04ca90

The introduction of the minimap screenshot feature appears to be duplicating functionality that will be used elsewhere for drawing the widgets within the minimap window; specifically, the GetMinimapOwner function is very likely to be replicating functionality used by the owner information overlay of the minimap window.

Expected result

We shouldn't have multiple codepaths that perform substantially similar functions.

@sauntheninja2
Copy link

@sauntheninja2 sauntheninja2 commented Apr 12, 2021

I would like to work on this issue but how would you eliminate duplication here?

Loading

@Phyrik
Copy link

@Phyrik Phyrik commented Jun 15, 2021

Hi, I'd also like to work on this if possible.

It looks like GetMinimapOwner in screenshot.cpp returns the Owner based on the tile given, and that is then used in MinimapScreenCallback to get the colour of the tile. This code seems to have been duplicated from GetSmallMapOwnerPixels in smallmap_gui.cpp, which returns the colour of the tile for the small map, with very little difference.

This could possibly be put into a new file, or an existing one, tasked with getting a colour for a specific world tile, which both the screenshot file and small map file could use instead.

I'm new to open source and OpenTTD development so please let me know if I got anything wrong or if there's a better solution!

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 15, 2021

I honestly suspect none of the devs really looked into it, as that most likely would also result in a solution :P But what you describe sounds good, but we won't really know till we see the code :D

I would say, just give it a spin, and make a Pull Request when you think it is done. We take it from there :)

As for new file or existing, I would go with an existing one. Finding a good place might be a bit tricky, but between the 500+ files we have I am sure it fits in one of them :D

Loading

@Phyrik
Copy link

@Phyrik Phyrik commented Jun 15, 2021

Thanks for the very quick response!

Yep, I'll try and work on a proper coded solution and open a PR when I think it's ready, as well as try to find somewhere the code could go.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants