More 3to4 testing results

[and3rson]

Godot version

4.0.dev (d8ec27cdc1)

System information

ArchLinux

Issue description

While test-migrating my project to 4.0 using latest Godot (master, d8ec27cdc1), I've found several more issues.

I'm willing to work on some (or even all) of those, but I first wanted to share it all in case someone else is already fixing some of these, or if something is as expected. :) So please consider this to be a tracking issue and let me know if I can take care of those.

• JSON code does not account the necessary indentation:

  # actual:
  
  def fn():
      if file.open("user://state.dat", File.READ) == OK:
  var test_json_conv = JSON.new()
  test_json_conv.parse(file.get_as_text())
          _config = test_json_conv.get_data()
      _config.merge(_default_config)
  
  # expected:
  
  def fn():
      if file.open("user://state.dat", File.READ) == OK:
          var test_json_conv = JSON.new()
          test_json_conv.parse(file.get_as_text())
          _config = test_json_conv.get_data()
      _config.merge(_default_config)
  

• Tween methods are not using Callable (tween_property, tween_callback)
• Lines like func start(a, b): ... are converted into new Thread.start() syntax. My suggestion is to convert .start only, not any start
• Quotes are inserted around signal name literals: await my_method().'my_signal'
• New get/set syntax uses tabs despite rest of the file is using spaces
• Connecting to signal with more than 1 bound arg breaks:

  # 3.x code:
  Warper.connect('done', $reload, 'set', ['disabled', false])
  # 4.x actual:
  Warper.connect('done',Callable($reload,'set').bind('disabled'),false])
  # 4.x expected:
  Warper.connect('done',Callable($reload,'set').bind('disabled',false))
  

• KinematicBody2D's set_motion_velocity is incorrect, should be set_velocity
• set_shader_param() should be set_shader_uniform(). I think 3to4 should also replace it in strings, since our project has, for example, a lot of things like death_tween.tween_property(Callback(G.renderer.material, "shader_param/aberration_strength"), 0.0, 0.25 * 4.0) which will become silently broken
• instance() should be instantiate()
• Button's pressed property is not a property anymore, but a signal. Should be migrated to button_pressed.
• func lock(): pass [...] self.lock() is treated as if it's a built-in image method: # TODOConverter40, image no longer require locking, `false` helps to not broke one line if/else, so can be freely removed
• assert message (2nd arg) is being commented out, which breaks code since it's required
• move_and_slide partially disregards node path:

  # 3.x code:
  $viewport_container/viewport/ball.move_and_slide(velocity, Vector3.UP)
  # 4.x actual:
  ball.set_motion_velocity(velocity)
  ball.set_up_direction(Vector3.UP)
  $viewport_container/viewport/ball.move_and_slide()  # <- this line is correct
  # 4.x expected:
  $viewport_container/viewport/ball.set_velocity(velocity)
  $viewport_container/viewport/ball.set_up_direction(Vector3.UP)
  $viewport_container/viewport/ball.move_and_slide()
  

• CharacterBody's get_slide_count should be get_slide_collision_count

Steps to reproduce

Not applicable

Minimal reproduction project

No response

[and3rson]

Tagging @KoBeWi, @rburing & @qarmin

[KoBeWi]

assert message (2nd arg) is being commented out, which breaks code since it's required

It's not required and is likely commented-out due to #47157

[qarmin]

instance is already handled in normal gdscript files, - https://github.com/godotengine/godot/blob/d8ec27cdc1a77f9516b4ee628437ddda814b5d6f/editor/project_converter_3_to_4.cpp#L1692 Llooks that you are using it inside tscn files, so additional custom rule like custom_rename(file_content, "instance()", "instantiate()"); should be added. Instance is valid keyword inside tscn files - https://github.com/godotengine/tps-demo/blob/dce79b7c6a8308d2e291634bcaa57216b87c0fd4/level/level.tscn#L40

Not sure about lock function and how often it is used in code, in random project which I checked it was not used ever once, but looks that this one rule can be removed.

pressed is quite common word so I'm not sure if it is possible to rename this without breaking other user functions

Anyway feel free to open PR to fix this problems - recently I created my own PR, because I wanted to do some refactor and now should be easier to modify code in converter.

[andunai]

instance is already handled in normal gdscript files, -

Could that be hacked around by using, say, instance( as a search string?

[qarmin]

It is possible(and recommended by me) to add as custom rule instance( or regex which will do same thing.

It is also possible(and highly not recommended by me) to hack easily gdscript_function_renames add instance( and similar directly to this array https://github.com/godotengine/godot/blob/21f6916ffc52d83ac57b4f412f3eb8d6b40afebe/editor/project_converter_3_to_4.cpp#L171 but I don't like this idea, because it would mix simple function renames with more advanced ones.

[and3rson]

Closing this since most items have been resolved, while "WONTFIX" ones are either too rare/unimportant or simply problematic to implement. Thanks for your attention, everyone!