Fix RDPipelineColorBlendState.attachments setter

[dalexeev]

Fixes the error

  Attempted to push_back a variable of type 'Array' into a TypedArray of type 'Object'.
  core/variant/array.cpp:269 - Condition "!_p->typed.validate(value, "push_back")" is true.

var pcbs = RDPipelineColorBlendState.new()
var attachment = RDPipelineColorBlendStateAttachment.new()
attachment.enable_blend = true
attachment.alpha_blend_op = RenderingDevice.BLEND_OP_ADD
attachment.color_blend_op = RenderingDevice.BLEND_OP_ADD
attachment.src_color_blend_factor = RenderingDevice.BLEND_FACTOR_SRC_ALPHA
attachment.dst_color_blend_factor = RenderingDevice.BLEND_FACTOR_ONE
attachment.src_alpha_blend_factor = RenderingDevice.BLEND_FACTOR_SRC_ALPHA
attachment.dst_alpha_blend_factor = RenderingDevice.BLEND_FACTOR_ONE
var attachments : Array[RDPipelineColorBlendStateAttachment] = [ attachment ]
pcbs.attachments = attachments

• I'm not sure if it's correct to give access to the internal array, user can update array content without calling the setter, see Packed array properties of noncustom resources can't be resized #80950.

CC @BastiaanOlij

[dalexeev]

Suggested change

	attachments.clear();
	attachments.append_array(p_attachments);
	attachments = p_attachments;

is_same(attachments, p_attachments) can be true, the code will not work in this case.

[BastiaanOlij]

Wonder why the original code was this way, thanks for fixing, looks great!

[dalexeev]

I marked this as a draft because I don't think the current solution is fully correct. Must be one of the following:

Option 1

void set_attachments(const TypedArray<RDPipelineColorBlendStateAttachment> &p_attachments) {
    attachments.clear();
    attachments.append_array(p_attachments);
}

TypedArray<RDPipelineColorBlendStateAttachment> get_attachments() const {
    return attachments.duplicate();
}

Option 2

void set_attachments(const TypedArray<RDPipelineColorBlendStateAttachment> &p_attachments) {
    attachments = p_attachments;
}

TypedArray<RDPipelineColorBlendStateAttachment> get_attachments() const {
    return attachments;
}

I checked the setters and getters of other non-Object properties of pass-by-reference types (arrays and dictionaries don't have signals to track external change, unlike resources). This is inconsistent.

[BastiaanOlij]

@dalexeev the pass by reference is only to prevent a parameter copy (and thus ending up with two copies being made), it still duplicates the content of the array.

I think option #2 is clearer, and as far as I can test works fine.

[dalexeev]

@BastiaanOlij Option 2 is the current option? Your comment confused me, sorry.

[BastiaanOlij]

@dalexeev I mean in relation to this:

I checked the setters and getters of other non-Object properties of pass-by-reference types (arrays and dictionaries don't have signals to track external change, unlike resources). This is inconsistent.

We're not storing a reference, we're only passing by reference to prevent an extra copy overhead.
So if you do:

pcbs.attachments = attachments
attachments.push_back(another_attachment)

There is no expectation that that pcbs sees the new attachment. The list of attachments has already been copied. It's not like a resource where a pointer to the original array is kept and changes to the array after assign still effect things.

So:

void set_attachments(const TypedArray<RDPipelineColorBlendStateAttachment> &p_attachments) {
    attachments = p_attachments;
}

TypedArray<RDPipelineColorBlendStateAttachment> get_attachments() const {
    return attachments;
}

Does exactly what we want and keeps the code concise.

[akien-mga]

So TL;DR, Bastiaan approved without hitting the "Approve" button ;)

[akien-mga]

Thanks!