Remove DEFVAL

[KoBeWi]

It doesn't do anything and gives false impression that it's needed.

[fire]

I'm not able to approve / review every single line of the pr, but I love the idea.

[KoBeWi]

I just used regex, so as long as it compiles it should be fine.

[Calinou]

Isn't DEFVAL() used for the --doctool class reference generation? Or did that change in master with the new method binding system?

[KoBeWi]

I just checked and it seems like the defaults are still properly documented.

[Xrayez]

Looks like it's a simple macro define which does nothing (at least in 3.2):

// class_db.h

#define DEFVAL(m_defval) (m_defval)

So at most this has readability purpose. But considering that a lot of Godot devs are already familiar with the binding usage, this seems redundant, so I think it makes sense to remove it. I'm for reducing verbosity since binding methods oftentimes take a lot of space wasted beyond 120 chars line.

[AndreaCatania]

I'm against this, because it makes the bindings much less clear, especially when you have a lot of variables and a lot of bindings.

[akien-mga]

I don't have a strong opinion, but I also tend to like having the explicit DEFVAL() to show what part of (usually pretty long and sometimes complex) the method bind call is the default value, to be able to visually match it with the arguments.

I also think that DEFVAL() makes it clear to new contributors how they can specify default values for their arguments. It's not obvious until you're very familiar with the binding system that default values from the C++ prototype will not be used in the bindings, and that they should be replicated in the bind_method call. Since many methods do have default arguments, any new contributor quickly stumbles upon such examples and can understand relatively intuitively what this is used for.

That being said I didn't know until yesterday that DEFVAL() was actually a no-op so it's still worth discussing.

[Xrayez]

I'm against this, because it makes the bindings much less clear, especially when you have a lot of variables and a lot of bindings.

@AndreaCatania I'm kind of surprised that you think this way because you've previously expressed that you're for reducing verbosity in Godot as a whole, see your own gist in https://gist.github.com/AndreaCatania/fdf9bd9942779ac9ec0c58be26393fd4. 😮

Unless I misunderstand something. One could say that using ClassDB::bind_method and ADD_PROPERTY is also more clear and explicit, but you choose to write that with EXPOSE macro, for instance.

TL;DR: don't act like reduz. 😛

[Xrayez]

That being said I didn't know until yesterday that DEFVAL() was actually a no-op so it's still worth discussing.

Well, it's nice to know about this, developers can now choose to omit DEFVAL in custom C++ modules in either case. 😛

I agree that it's more explicit, but that sort of explicitness made refactoring a hell in the past for me. It makes it more difficult to re-arrange default value, and it's easy to make a mistake since DEFVAL can take more characters than the actual default value (most of the time). Rather, it's not really about the number of characters but similarity/being monotone which makes it more difficult to distinguish. C++ default values don't have these issues because those values are matched exactly with parameter names which are more explicit than DEFVAL.

Also, a more explicit way to define parameters would be to wrap them in some kind of ARG macro:

ClassDB::bind_method(D_METHOD("method", "arg0", ARG("arg1", 37), ARG("arg2", "Hello")), &Class::_method);

As you see, this can achieve explicitness and clarity (yes, explicitness does not necessarily mean clarity).

[AndreaCatania]

@Xrayez I'm all for improving the syntax and make it less verbose, but I've the feeling that just doing this change is not enough. Rather, it's going to make things look more complex, IMO.

Taking this as reference: https://github.com/godotengine/godot/blob/master/core/io/image.cpp#L3088

ClassDB::bind_method(D_METHOD("resize_to_po2", "square", "interpolation"), &Image::resize_to_po2, DEFVAL(false), DEFVAL(INTERPOLATE_BILINEAR));
ClassDB::bind_method(D_METHOD("resize", "width", "height", "interpolation"), &Image::resize, DEFVAL(INTERPOLATE_BILINEAR));
ClassDB::bind_method(D_METHOD("detect_used_channels", "source"), &Image::detect_used_channels, DEFVAL(COMPRESS_SOURCE_GENERIC));
ClassDB::bind_method(D_METHOD("compress", "mode", "source", "lossy_quality"), &Image::compress, DEFVAL(COMPRESS_SOURCE_GENERIC), DEFVAL(0.7));
ClassDB::bind_method(D_METHOD("compress_from_channels", "mode", "channels", "lossy_quality"), &Image::compress_from_channels, DEFVAL(0.7));

ClassDB::bind_method(D_METHOD("resize_to_po2", "square", "interpolation"), &Image::resize_to_po2, false, INTERPOLATE_BILINEAR);
ClassDB::bind_method(D_METHOD("resize", "width", "height", "interpolation"), &Image::resize, INTERPOLATE_BILINEAR);
ClassDB::bind_method(D_METHOD("detect_used_channels", "source"), &Image::detect_used_channels, COMPRESS_SOURCE_GENERIC);
ClassDB::bind_method(D_METHOD("compress", "mode", "source", "lossy_quality"), &Image::compress, COMPRESS_SOURCE_GENERIC, 0.7);
ClassDB::bind_method(D_METHOD("compress_from_channels", "mode", "channels", "lossy_quality"), &Image::compress_from_channels, 0.7);

I feel like that the last parameters may be miss understood for ""additional config"" on the binding, while DEFVAL make it obvious for anyone.

[akien-mga]

Still not sure which way to go between keeping DEFVAL for readability or removing it because it's a no-op. I tend towards the former but I wouldn't oppose the latter if that's the preference of most contributors.

Here's a couple pathological examples where I think the cosmetic DEFVAL helps make things clearer:

ClassDB::bind_method(D_METHOD("draw_multiline_string", "font", "pos", "text", "align", "width", "max_lines", "size", "modulate", "outline_size", "outline_modulate", "flags"), &CanvasItem::draw_multiline_string, DEFVAL(HALIGN_LEFT), DEFVAL(-1), DEFVAL(-1), DEFVAL(-1), DEFVAL(Color(1, 1, 1)), DEFVAL(0), DEFVAL(Color(1, 1, 1, 0)), DEFVAL(TextServer::BREAK_MANDATORY | TextServer::BREAK_WORD_BOUND | TextServer::JUSTIFICATION_KASHIDA | TextServer::JUSTIFICATION_WORD_BOUND));
ClassDB::bind_method(D_METHOD("draw_char", "font", "pos", "char", "next", "size", "modulate", "outline_size", "outline_modulate"), &CanvasItem::draw_char, DEFVAL(""), DEFVAL(-1), DEFVAL(Color(1, 1, 1)), DEFVAL(0), DEFVAL(Color(1, 1, 1, 0)));

Without DEFVAL for comparison:

ClassDB::bind_method(D_METHOD("draw_multiline_string", "font", "pos", "text", "align", "width", "max_lines", "size", "modulate", "outline_size", "outline_modulate", "flags"), &CanvasItem::draw_multiline_string, HALIGN_LEFT, -1, -1, -1, Color(1, 1, 1), 0, Color(1, 1, 1, 0), TextServer::BREAK_MANDATORY | TextServer::BREAK_WORD_BOUND | TextServer::JUSTIFICATION_KASHIDA | TextServer::JUSTIFICATION_WORD_BOUND);
ClassDB::bind_method(D_METHOD("draw_char", "font", "pos", "char", "next", "size", "modulate", "outline_size", "outline_modulate"), &CanvasItem::draw_char, "", -1, Color(1, 1, 1), 0, Color(1, 1, 1, 0));

(Put them as inline code on purpose as code blocks add a scrollbar which defeats the aim of this demonstration :))

Since default values apply to the rightmost arguments, searching DEFVAL in an IDE (or here in your web browser) can help visually grep how many default arguments are passed for a given methods which has more arguments than default values (like draw_char above, you can see quickly that it has 5 default values thus 5 optional arguments - so up to "next").

For draw_multiline, the DEFVAL(TextServer::BREAK_MANDATORY | TextServer::BREAK_WORD_BOUND | TextServer::JUSTIFICATION_KASHIDA | TextServer::JUSTIFICATION_WORD_BOUND) is pretty extreme but it's also useful to have it clearly enclosed.

[fire]

The arguments are cosmetic DEFVAL helps make things clearer and it is the current status. So the status quo has an advantage over the alternative which making things unclear and create a lot of noise in commits.

[Xrayez]

The arguments are cosmetic DEFVAL helps make things clearer and it is the current status. So the status quo has an advantage over the alternative which making things unclear and create a lot of noise in commits.

How about making DEFVAL accept multiple arguments, like DEFVAL("value", Color(1, 1, 1), true)? Allows to eliminate redundant DEFVAL on a single line, should improve readability significantly here.

[Faless]

How about making DEFVAL accept multiple arguments, like DEFVAL("value", Color(1, 1, 1), true)? Allows to eliminate redundant DEFVAL on a single line, should improve readability significantly here.

Preprocessor macros don't support variable arguments. Erm... my bad, I thought they didn't, but they actually do :/

[reduz]

As others have expressed, I also prefer the way things are right now for better readability. Given there is no consensus on this I will be closing the PR.