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

Feature: Minimap screenshot #7817

Merged
merged 1 commit into from Jan 4, 2020
Merged

Feature: Minimap screenshot #7817

merged 1 commit into from Jan 4, 2020

Conversation

telk5093
Copy link
Contributor

@telk5093 telk5093 commented Nov 2, 2019

This command is introduced at this tt-forum post and already implemented in JGRPP as minimap owner <file_name>

This feature extends screenshot command to take top-viewed screenshot as screenshot topview.
Its output looks like this (from title game):
minimap

And adds a dropdown menu:
image

eg)
screenshot minimap
screenshot minimap <filename>

Most part of this code is not from me, but from here.
I have no idea if I dare could make a PR although its original author is not me.
Please let me know if it is not allowed or there is any problem.

Btw, it is quite useful command to show users server's status via website.
I am using JGRPP's minimap command to show my server's status in my website

JGRennison added a commit to JGRennison/Upstream-OpenTTD that referenced this pull request Nov 2, 2019
@LordAro
Copy link
Member

LordAro commented Nov 3, 2019

I like it, but I feel like it would be better implemented as a separate "screenshot mode". This would ensure no issues with overwriting files. Shouldn't mean many changes in the core functionality you've implemented

@telk5093 telk5093 changed the title Feature: minimap command Feature: Topview screenshot (a.k.a minimap) Nov 17, 2019
@telk5093
Copy link
Contributor Author

I think you intended like this new code. Now it is unifyed with screenshot command.
Take a look and please let me know something is wrong or any your advices.

@telk5093 telk5093 force-pushed the minimap branch 2 times, most recently from 1ad5f9e to 94d2c9b Compare November 17, 2019 07:27
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

I think "minimap" might be a better name for it than "topview", even if the "viewing angles" are different between the ingame minimap and the screenshot

src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/toolbar_gui.cpp Outdated Show resolved Hide resolved
@telk5093 telk5093 changed the title Feature: Topview screenshot (a.k.a minimap) Feature: Minimap screenshot Nov 23, 2019
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

This looks like a lot of comments, but I'm pretty sure most of them are easily fixable. Getting close!

src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.h Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
@telk5093
Copy link
Contributor Author

telk5093 commented Jan 4, 2020

I applied recent /pull/7550's changes, please look around. Sorry for my pool coding skill beforehand :)

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Seem to be some remaining changes in toolbar_gui.cpp - those should be removed

Also remember to run the relevant GS generation scripts - script/api/generate_widget.sh

@telk5093
Copy link
Contributor Author

telk5093 commented Jan 4, 2020

Thanks for your advices, please check if I've done correctly.

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

Successfully merging this pull request may close these issues.

None yet

3 participants