godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Allow overriding built-in setter and getter methods with a script #344

Closed lukostello closed 2 years ago

lukostello commented 4 years ago

Describe the project you are working on: I am working on a puzzle game involving rotation which uses similar mechanics to rubiks cubes and combination locks.

Describe the problem or limitation you are having in your project: In my levels I want several objects to wrap around the screen pacman style. I'd like to make a setter like this

func set_translation(value : Vector3):
    value.x = fposmod(value.x, level_width)
    translation = value

I'd also like to make to ensure my rotations don't go over TAU to prevent overflow so I want to make a setter to ensure it stays in that range.

func set_rotatation(value : Vector3):
    value.y = fposmod(value.y, TAU)
    rotation = value

but it seems transform methods cannot currently be overriden.

Describe how this feature / enhancement will help you overcome this problem or limitation: The feature I want is the ability to override set_rotation and set_translation functions. Such that when I use translation = Vector3 or translation.x = value it can make the appropriate adjustments. It seems to me like there was always the intention that we should be able to do this but something got over looked and we can't. I don't know how pervasive this is but it seems this is not possible using spatial or node2D for transform methods, it could be a bigger problem than that however. There could be many more setters/getters from parent functions that are are not able to be overridden but should.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work: We would just make getters and setters just like the parent classes and they would be overriden such that when you do rotation.y +=2.5 * TAU for example it would call the setter I set above.

Describe implementation detail for your proposal (in code), if possible: I don't know c++ well enough yet to tell exactly why this is a problem in the first place.

If this enhancement will not be used often, can it be worked around with a few lines of script?: For every variable of the spatial class I want to have a setter for, I could make a second variable like psuedo_rotation. Then use a setter on that, then on physics_process() set rotation = psuedo_rotation. But then I don't get all the functionality the actual rotation has from the spatial class and it wouldn't be perfectly synced especially when it comes to changes triggered by input events. It should also be totally unnecessary to make an assignment every iteration, all the tools are there just not being used. Alternatively I could do the setter manually every time I set the transform information but that is extremely tedious, unreadable, I might forget an instance or implement it differently on accident.

Is there a reason why this should be core and not an add-on in the asset library?: its already there I think someone must have accidentally not made it overridable.

bojidar-bg commented 4 years ago

Note that an assignment every frame is almost nothing. Checking if the script has a function defined every time rotation or translation is changed, on the other hand, is somewhat expensive.

lukostello commented 4 years ago

Note that an assignment every frame is almost nothing. Checking if the script has a function defined every time rotation or translation is changed, on the other hand, is somewhat expensive.

How could that be the case considering not every frame a rotation/translation occurs? If your argument is valid then you are supposing that setter/getters are an inefficient tool in all cases. I doubt that is the case otherwise they would have never be used in the first place. If there is a case where they are more efficient then I don't see how my instance is any different.

Calinou commented 2 years ago

We discussed this in a proposal review meeting. We found that this proposal lacked community support, but also that it could introduce too much "magic" and be difficult to debug. Therefore, this won't be implemented.