-
-
Notifications
You must be signed in to change notification settings - Fork 22k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CollisionShape{2D,3D}.debug_color
inconsistencies
#100093
Fix CollisionShape{2D,3D}.debug_color
inconsistencies
#100093
Conversation
|
||
Color CollisionShape2D::_get_default_debug_color() const { | ||
SceneTree *st = SceneTree::get_singleton(); | ||
return st ? st->get_debug_collisions_color() : Color(0.0, 0.0, 0.0, 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to keep Color()
(black) instead of transparent black as the fallback?
If using collision shapes without a scene tree, a black shape is probably better than a transparent one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is debatable. On the one hand, at least you'll see the shape. On the other hand, you might get a bunch of indistinguishable black rectangles, squares, spheres, etc. But in any case, this is a very hypothetical scenario, the vast majority of users use the standard SceneTree
, and do not implement their own MainLoop
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This transparent black already got noticed by a user of my voxel terrain module. They then though collision didn't work, but then we figured out it's because of the default value. Our system doesn't use nodes so we then had to add code to grab the debug_color from SceneTree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer transparent or black color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use a fallback value that's actually a good option, like the current default?
1837: debug_collisions_color = GLOBAL_DEF("debug/shapes/collision/shape_color", Color(0.0, 0.6, 0.7, 0.42));
I think it's fine if hardcoded and not following user config as it's for more niche use cases, though from the report we got so far, more users seem to be affected by the transparent fallback that we thought.
Is this an issue this PR already addresses though, making all projects using SceneTree have the proper color (as opposed to only projects using shape nodes)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be for a better default too, although having skimmed over some code related to it, I think it's been used as a special value to mean "null". Maybe that should be checked as well
f8f5767
to
4b73862
Compare
4b73862
to
8bf2afd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already better than the status quo, but maybe the fallback color could be something colored and semi-transparent like the previous default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge as is for now and see if a different default is useful after dev7.
Thanks! |
CollisionShape3D
's propertydebug_color
should not be saved in .tscn file #100088.CC @BattyBovine