Update Camera 2D to fix and remove some notifications

[ghost]

This simple pull request will fix : #28492
By removing the if statement that stops the camera from updating using NOTIFICATION_TRANSFORM_CHANGED .
And removing the NOTIFICATION_INTERNAL_PROCESS as it has no use.
As well as NOTIFICATION_PROCESS

[lawnjelly]

I did write a little about it here:
#28492 (comment)

as this is basically just a PR on:
#28492 (comment)

It may be reasonable to do this, but I agree with reduz .. we need to take care in this area. There needs to be a lot more research into why the if statement was there in the first place, and what the knock on effects of such a change will be, before merging such a change. Perhaps you have done this, but it needs to be documented here. Also, how expensive is the _update_scroll?

It is very possible that by fixing this in this way will introduce other bugs, or have bad effects on performance, or be compatibility breaking with existing games. For instance objects updated before the camera in the scene tree might see it one position, and objects updated afterward see it in another. Order of operations like this can be a minefield.

Overall though I don't know enough about this area of order of operations to give a sensible review. You would have to discuss it with reduz, who probably originally wrote this.

[ghost]

@lawnjelly

As I stated in #28492
My debugging shows that the _update_scroll after the if statement never get called.
That mean its broken in that state.

Also, how expensive is the _update_scroll?

It is expensive, that's why We refused to use force_update_scroll()

why the if statement was there in the first place

My theory is that reduz tried to call _update_scroll only when the camera changes position.
by masking NOTIFICATION_TRANSFORM_CHANGED using that if statement.

my evidence is he checks if is_processing_internal is off.
(we think he should check it for on)

We did a test to see how many times does:
NOTIFICATION_INTERNAL_PROCESS and NOTIFICATION_TRANSFORM_CHANGED get called.

they are called the same number of times.

So maybe there is no performance problems.

only reduz can confirm our hypothesis.

NOTE : Also we do think this need to be changed, to update only when the camera, or one of its parents is moved, not when something else moved, or when a frame is generated.

[lawnjelly]

We should probably revisit this PR (or at least this general area), as it should fix the remaining problems with camera snapping. We just need to make sure it doesn't cause any regressions due to the order of updates.

I've tried it on a few projects and it seems okay - if we are to try this I suspect we will need at least a temporary project setting to enable / disable it in case of regressions, so this would be worth adding to the PR (default to on, as is the preference for testing these things as it is more likely to get more eyes on it).

[lawnjelly]

Actually I woke up this morning realising why it is done this way, and have a better way of solving this, same basic idea though. Will prepare a PR.

[ghost]

Thanks to @lawnjelly #46697

We could close this PR.

Some notes
I want to side note the major difference between this PR and #46697

-This PR deletes some notification cases and letting the _update_scroll called every time, while his PR keeps the notification cases only changes the condition that allow _update_scroll to be called.

Hopefully his change will not result in a bug.

As @reduz quoted in his PR:

Yes, processing is enabled wrongly, so this is not a complete fix, it may break during process.

Thank you.

[lawnjelly]

Yup there's I've got another PR in to improve the process logic: #46717, but I'll do this in consultation with the others.

It would still run ok as is I think, as the fixed version will call _update_scroll in more cases, not less, but the process logic could do with clearing up. Reduz was keen to make sure the _update_scroll didn't run in the transform notification when smoothing was off, I wasn't 100% sure on this but his explanation sounded reasonable.

I'm also a fan of removing the other notification cases if not necessary, on the other hand with the fixed logic, they may not get called at all anyway if internal_process and internal_physics_process are switched off. Yes on reflection I think this is solved already with the logic PR.

Also, I haven't timed or profiled but I'm not completely sure that _update_scroll is actually that expensive in the end.

I actually first wrote a more complex PR #46569 for ensuring that _update_scroll was called a maximum of once per frame based on your idea of it being expensive, using a dirty flag. If timing does show it is expensive we could revisit this at a later date.