Skip to content
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

add Packed*Array::pop_back #83000

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add Packed*Array::pop_back #83000

wants to merge 1 commit into from

Conversation

bend-n
Copy link
Contributor

@bend-n bend-n commented Oct 8, 2023

closes (what i need) of godotengine/godot-proposals#4637

@bend-n bend-n requested review from a team as code owners October 8, 2023 13:21
@AThousandShips
Copy link
Member

AThousandShips commented Oct 8, 2023

This is pretty trivial, just sugar for arr.resize(arr.size() - 1), I don't see the need for this

pop_back on the other hand

See here

@bend-n
Copy link
Contributor Author

bend-n commented Oct 8, 2023

I'm not sure how to correctly implement pop_back.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 8, 2023

See the implementation in Array:

Variant Array::pop_back() {
ERR_FAIL_COND_V_MSG(_p->read_only, Variant(), "Array is in read-only state.");
if (!_p->array.is_empty()) {
const int n = _p->array.size() - 1;
const Variant ret = _p->array.get(n);
_p->array.resize(n);
return ret;
}
return Variant();
}

Would instead involve grabbing the last value, and then resizing back

@bend-n
Copy link
Contributor Author

bend-n commented Oct 8, 2023

Is there no reason we have remove_at and not pop_at, then?

@AThousandShips
Copy link
Member

Unsure, but could be because it has not been considered needed for general use of Vector

@AThousandShips
Copy link
Member

Note that Array doesn't have remove_back, only pop_back

I'd argue that having a method that just does:

if not arr.is_empty():
    arr.resize(arr.size() - 1)

Is not needed, especially not added to a core component like Vector

@bend-n
Copy link
Contributor Author

bend-n commented Oct 8, 2023

Im skeptical if

T pop_back() {
	ERR_FAIL_COND(_cowdata.size() == 0);
	T tmp = *_cowdata.get(_cowdata.size() - 1);
	_cowdata.resize(_cowdata.size() - 1);
	return tmp;
}

would work with gdscript and all

@AThousandShips
Copy link
Member

AThousandShips commented Oct 8, 2023

That's why it should probably error for Packed*Array

T pop_back() {
	ERR_FAIL_COND_V(_cowdata.is_empty(), T());
	T tmp = *_cowdata.get(_cowdata.size() - 1);
	_cowdata.resize(_cowdata.size() - 1);
	return tmp;
}

Alternatively write a dedicated handler for just Typed*Array, like is done for PackedByteArray for some complex functionality

@bend-n bend-n changed the title add Packed*Array::remove_back add Packed*Array::pop_back Oct 9, 2023
@bend-n
Copy link
Contributor Author

bend-n commented Oct 9, 2023

I wonder if i ought to add pop_at, pop_front as well.

@AThousandShips
Copy link
Member

I'd take that as a separate thing, also Vector doesn't have push_front so would be unbalanced to add the pop_front

@bend-n bend-n force-pushed the bremove branch 4 times, most recently from 209461f to d24e5f9 Compare October 9, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants