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

Camera guides - action safe, title safe and rule of thirds (custom grid) #6065

Open
wants to merge 7 commits into
base: 1.4_maintenance
Choose a base branch
from

Conversation

BrianHanke
Copy link
Contributor

Hi John, here's an official PR with my preliminary camera guide code. It seems to be working well in practice, but I'm sure it can be much more elegant and flexible. I made notes tagged with BHGC talking about what I did and what I'm unsure about. The "Rule of Thirds" guide can draw any number of grid lines, just set div_h and div_v as desired. The action safe and title safe ones need to have an area, not dimensions, of 90% and 80%, hence the square root stuff.

Added camera guide code.
Added camera guide code.
No clue how that "nhea" got in there!
Change to range loop and eliminate white space
Fix whitespace
@danieldresser-ie
Copy link
Contributor

I don't think I can give much meaningful feedback here - I've tagged John as reviewer.

The one note I would make for setting this up for review is that currently the commit history doesn't seem very helpful - if the commits aren't split into separate logical chunks of work, it might make sense to just squash everything. ( The hard part is when you've spent a week working on 20 different iterations that end up contributing to 2 or 3 logical commits ... it can take 20 minutes to regroup things into a logical grouping, but it does make things a lot easier to review. In this case though, it seems like everything is pretty related, and it would probably be fine to squash it all into one ).

@BrianHanke
Copy link
Contributor Author

@danieldresser-ie Gotcha. I'm still a Github noob so I have to learn how to make things organized on that front. In this case the entire history is just me making it pass the checks. The actual functional code hasn't changed. I got worried there was a problem when it complained about whitespace and the Linux builds failed because of my C++ syntax. Should I do anything to fix the situation now or can I leave it as is?

@danieldresser-ie
Copy link
Contributor

John can review it for now just by looking at the final result, though we do always like to clean up the history at some point before merging ( my work in progress commit history on complex features is usually an absolute disaster that needs a lot of cleanup ). I don't know if you're using github on the command line, or some sort of GUI, but for these sorts of fix commits, what I would do is git rebase -i 1.4_maintenance ( Interactive rebase of my branch onto the base branch ), and then in the text editor it brings up, change the pick to f ( for fixup ) for all the followup commits. That squashes the followups as fixes to the previous commit, so you end up with just one commit ( so it looks like you got it right the first time :P ).

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.

2 participants