godotengine / godot-proposals

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

Make Area2D just for detecting collisions, add a new node for physics and audio overrides #2234

Closed AaronRecord closed 3 years ago

AaronRecord commented 3 years ago

Describe the project you are working on

Snake but it's a competitive 2d top-down shooter

Describe the problem or limitation you are having in your project

I'm using Area2d's for the collision detection in my game. The problem is Area2D's have a bunch of unnecessary properties that go unused most of the time (for overriding physics and audio settings).

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I think that the Area2D node should be simplified so that it's only purpose is to detect collisions, since most of the time that's all that anyone needs. For the times when someone does need to override the physics or audio 2d settings, I think there should be a separate node for that (that inherits from Area2D), called something like "OverrideArea2D" (or maybe we should also separate the physics and audio overrides into separate nodes, like "PhysicsOverrideArea2D" and "AudioOverrideArea2D"). I think this will be beneficial because it will declutter the inspector/documentation/api for Area2D, since most of the time Area2D's are just being used to detect collisions.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

Because Area2D is part of core

Calinou commented 3 years ago

I'm not sure if it's worth the effort to split a node into two just to provide a simpler version. As far as I know, we don't do this in Godot anywhere else. For example, KinematicBody is not a simpler version of RigidBody.

AaronRecord commented 3 years ago

@Calinou I feel like it's like how VisibilityNotifier and VisibilityEnabler are seperate nodes. You could just export a bool called something like "enabled_when_visible" on VisibilityNotifier and remove the VisibilityEnabler node, but that's not VisibilityNotifier's job (I think it's called the single responsibility principle, where each class should only be responsible for one thing, but please correct me if I'm wrong).

I also just realized this is also relevant for Area3D, should I edit this is issue to make it about both? Or should that be a separate issue?

KoBeWi commented 3 years ago

This could be done, but unless it improves performance, it's not really useful change.

AaronRecord commented 3 years ago

@KoBeWi I mean it would save a few kb of memory if you had a bunch of areas... I mean it's an optimization, but one that'll have barely any impact. I just thought it didn't really make sense that the majority of the space in the inspector is taken up by properties that most of the time people don't even want, especially since I thought it'd be pretty easy to just make a new node for the times when you do want to override the physics and audio settings. Like in my snake game I'm using an area for the snake's collision, and it just doesn't make sense to have physics and audio override settings in there. If you don't think it makes sense to change the node, then I think at least putting the physics override settings in their own tab that's hidden by default like the audio override settings would be an improvement.

YuriSizov commented 3 years ago

This seems like a UX problem that can be solved by a better grouping of properties.

AaronRecord commented 3 years ago

This seems like a UX problem that can be solved by a better grouping of properties.

That would be an improvement for sure :) My reasoning with suggesting splitting Area2D into 2 nodes is because it feels to me like Area2D is 2 nodes squished into one; one for collision detection and one for local physics (and audio) overrides. If you're using an area2d to override local physics settings, then the current setup works fine, but if all you need is some basic collision detection then all the properties and functionality relating to overriding local physics settings feel totally unnecessary.

Edit: As an example, in my snake game, I have a food scene/class that extends Area2D. I don't need or want physics and audio override settings as part of my food class, I'm just using Area2D so that I can detect when they collide with the player, but I can't remove them because this functionality is built into Area2D (and not a separate node that inherits it).

AaronRecord commented 3 years ago

I still believe that it would make more sense to have a node just for collision detection and a separate one for local physics and audio overrides, but if most people disagree, then I think better organizing the inspector would be a good compromise. It could look something like this:

Shadowblitz16 commented 3 years ago

I actually wanted to make this exact suggestion. The only difference to mine is suggesting a area and physics body hybrid as well

I think this issue could be fixed by supplying a new physics parameters resource to the area2d instead of having all the properties being part of the area2d. this way you would be able to wrap it out if needed.

AaronRecord commented 3 years ago

I was reading this, and in it reduz says "Nodes do exactly one thing, so it's very easy to understand what a scene does by just looking at it.". I think this sums up my biggest complaint with how areas are currently set up, right now areas are doing 3 things, collision detection, local physics overrides and local audio overrides, which makes it feel especially clunky if all you want is collision detection.

AaronRecord commented 3 years ago

One problem with this solution (if Area2D was split into an AudioOverrideArea2D and PhysicsOverrideArea2D) is if you need an area with both audio and physics overrides then you would need to create 2 different areas with the same collision box, which could be kind of annoying. I think maybe a better way to implement what I wanted would be to give Area array of AreaOverride resources (which physics and audio override resources would inherit). In the future a post processing override resource could also be added. Maybe this isn't actually better than the current setup, but it's at least an idea. Or maybe just have audio and physics overrides be part of the same node that inherits Area.

Shadowblitz16 commented 1 year ago

@AaronRecord was this proposal dropped? I would still like to see the extra properties put into their own node that extends from Area2D and Area3D