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 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 force-pushed the telk5093:minimap branch from f5502ba to 37916f1 Nov 17, 2019
@telk5093 telk5093 changed the title Feature: minimap command Feature: Topview screenshot (a.k.a minimap) Nov 17, 2019
@telk5093
Copy link
Contributor Author

@telk5093 telk5093 commented Nov 17, 2019

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 telk5093:minimap branch 2 times, most recently from 1ad5f9e to 94d2c9b Nov 17, 2019
Copy link
Member

@LordAro LordAro left a comment

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 force-pushed the telk5093:minimap branch from 94d2c9b to 46c38af Nov 23, 2019
@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

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 telk5093 force-pushed the telk5093:minimap branch from c0dc67a to 86c97a9 Jan 4, 2020
@telk5093
Copy link
Contributor Author

@telk5093 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

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 telk5093 force-pushed the telk5093:minimap branch from 86c97a9 to b5a21a9 Jan 4, 2020
@telk5093
Copy link
Contributor Author

@telk5093 telk5093 commented Jan 4, 2020

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

@LordAro
LordAro approved these changes Jan 4, 2020
@nielsmh nielsmh merged commit e04ca90 into OpenTTD:master Jan 4, 2020
8 checks passed
8 checks passed
OpenTTD CI Build #20200104.2 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.