Skip to content

Conversation

DelinWorks
Copy link
Contributor

@DelinWorks DelinWorks commented Dec 23, 2022

Describe your changes

when changing the size of the game windows the camera's zoom resets to 1, the game code that sets the size of the camera has to be set again to mitigate this, this pr fixes this issue.

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • I have checked readme and add important infos to this PR (if it needed).
  • An improved cpp-test/lua-test is not needed (explain why not, thanks).
    it's a core feature that presumably doesn't need to have a test

@rh101
Copy link
Contributor

rh101 commented Dec 23, 2022

Wouldn't there be a need to have camera views that are not linked to the current game perspective, since some of the changes in this PR seem to force all cameras to be the same projection as the base view set in the director? What reason is there to force all cameras to have the same type of view perspective?

An example of a camera using a different perspective would be a 3D game that requires a 2D camera to be implemented, such as an overhead map.

@DelinWorks
Copy link
Contributor Author

DelinWorks commented Dec 24, 2022

this is how cameras used to work before i added the change that made them have their own perspectives, which didn't bring any benefit.

@rh101
Copy link
Contributor

rh101 commented Dec 24, 2022

this is how cameras used to work before i added the change that made them have their own perspectives, which didn't bring any benefit.

I wasn't aware that they worked that way before, but now my question is, why are you reverting the change? Has it something to do with this specific zoom issue? If it isn't related to the zoom fix, then leave that change out of this PR, and create a new PR for reverting the change to the camera perspective. It will allow others to have a say regarding the change, in case it is of any benefit to them.

@DelinWorks
Copy link
Contributor Author

DelinWorks commented Dec 24, 2022

i recently added the feature and it has no impact on anything really, no function or core feature accesses the camera's perspective, but the director perspective, if you want i could rename the pull because making a whole new pr for just two lines isn't logical

@rh101
Copy link
Contributor

rh101 commented Dec 24, 2022

if you want i could rename the pull because making a whole new pr for just two lines isn't logical

The size of the changes in a PR are not relevant, and as a guideline, the less complex a change, the better, as it makes the review process and code merging much easier; it also makes it easier to spot any problems with the changes.

If any issue does come up due to a commit, then having that commit linked to a specific PR can help in understanding why it was done in the first place, and rolled back as a whole unit if required. Use your good judgement for things like this, and as an example, simple fixes, like changes to white-spaces etc, do not require their own PR.

There are a lot of resources online that you can review regarding why this is important.

So, yes, it is preferred to not add unrelated code changes to this PR, and if required, make a new PR with the camera perspective changes.

@DelinWorks DelinWorks changed the title [BUG] Fix camera applyZoom() when window size changes [BUG] Fix camera applyZoom() when window size changes and remove unnecessary projection code Dec 24, 2022
@rh101
Copy link
Contributor

rh101 commented Dec 24, 2022

I get that changing the title seems like a work-around, but for future reference, the least that should be done is to add unrelated changes as separate commits, since it's straightforward to revert single commits.

@DelinWorks
Copy link
Contributor Author

DelinWorks commented Dec 24, 2022

Good idea @rh101 I'm not at my computer right now so a simple workaround was to change the title in my phone, but for the future this will be done no worries!

@DelinWorks
Copy link
Contributor Author

like that @rh101? it was kinda easy on my phone

@rh101
Copy link
Contributor

rh101 commented Dec 24, 2022

Yes, that's exactly it. Makes it easier to revert issues if any arise when they're contained to individual commits.

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.

3 participants