Fix potential infinite loop when calculating tile editor zoom level

[jsjtxietian]

Fixes #86496

[VetusChronos]

I made some modifications to the code as I noticed some potential issues, especially regarding zoom limits. Below, I describe the main ones:

Points of Divergence:

1. Consideration of Zoom Limits:

   • Changes in this PR: It does not explicitly consider the maximum and minimum zoom limits during the zoom adjustment loops.
   • My changes: Implement precise checks for the maximum and minimum zoom limits, avoiding infinite loops and ensuring that the zoom does not exceed these limits.

2. Zoom Adjustment Logic:

   • Changes in this PR: It uses loops to increase or decrease the zoom until reaching the desired tile size, without explicitly considering the zoom limits.
   • My changes: Considers whether the tile size multiplied by the maximum zoom is smaller than the desired size. It uses loops only when necessary, with checks for the zoom limits.

3. Handling Extreme Conditions:

   • Changes in this PR: It does not explicitly address scenarios where the original tile size multiplied by the maximum zoom is smaller than the desired size.
   • My changes: Specifically handles these extreme conditions, ensuring proper zoom adjustment even in situations where the initial tile size multiplied by the maximum zoom is smaller than the desired size.

My Code - I tested it locally and it worked without any apparent errors:

// Set the default zoom value.
int default_control_y_size = 200 * EDSCALE;

float target_tile_size = default_control_y_size / editor_zoom_widget->get_zoom();
Vector2 original_tile_size = tile_set->get_tile_size();
Vector2 current_tile_size = original_tile_size * editor_zoom_widget->get_zoom();

// Checks if the multiplication of the tile size by the maximum zoom is less than the default size
if (original_tile_size.y * editor_zoom_widget->get_max_zoom() < default_control_y_size) {
  // Adjusts the zoom to the maximum possible without exceeding the desired standard size
  float new_zoom = editor_zoom_widget->get_zoom() * (default_control_y_size / (original_tile_size.y * editor_zoom_widget->get_max_zoom()));
  if (new_zoom >= editor_zoom_widget->get_max_zoom()) {
  	editor_zoom_widget->set_zoom(editor_zoom_widget->get_max_zoom());
  } else {
  	editor_zoom_widget->set_zoom(new_zoom);
  }
} else {
  // Checks if the current tile size is smaller or larger than the desired size
  if (current_tile_size.y < target_tile_size) {
  	while (current_tile_size.y < target_tile_size && editor_zoom_widget->get_zoom() < editor_zoom_widget->get_max_zoom()) {
  		editor_zoom_widget->set_zoom(editor_zoom_widget->get_zoom() * 1.1);
  		current_tile_size = original_tile_size * editor_zoom_widget->get_zoom();
  	}
  } else if (current_tile_size.y > target_tile_size) {
  	while (current_tile_size.y > target_tile_size && editor_zoom_widget->get_zoom() > editor_zoom_widget->get_min_zoom()) {
  		editor_zoom_widget->set_zoom(editor_zoom_widget->get_zoom() * 0.9);
  		current_tile_size = original_tile_size * editor_zoom_widget->get_zoom();
  	}
  }
}

_zoom_changed();

I'm not sure if the issues I've highlighted are really important to emphasize, but I made that comment just in case and to check for problems.

This is my first time commenting on an open-source project, so I hope I haven't done anything strange or wrong. Have a great day!

[jsjtxietian]

@VetusScientia Thank you very much for your suggestion, invaluable.

I guess we can ask for some advice from @groud since he is the author of this code.

Also, feel free to take my pr and work based on it.

[groud]

Alright, sorry for the late review, and thanks for the contribution.

While @VetusScientia's one is, like, more "correct", it would add a lot of LoC for limited gains IMO. I think the approach by @jsjtxietian is the simplest one. I think the current state of the PR is acceptable.

This is my first time commenting on an open-source project, so I hope I haven't done anything strange or wrong. Have a great day!

You did perfectly fine ! :)

[groud]

Needs the comment by @AThousandShips 's addressed, but aside from that, LGTM.

[VetusChronos]

You did perfectly fine ! :)

Thank you very much, @Ground! I'm still studying Godot's source code, meaning I'm still a noob 😄

While @VetusScientia's one is, like, more "correct", it would add a lot of LoC for limited gains IMO.

But the current state of the PR is indeed simpler and works well. I hope to be able to contribute better in the future!

Have a great day!

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.2.2.