aryeramaty/blender
0
0
Fork 1
forked from blender/blender
Code Pull Requests Activity

davidhaver-WIP-realize-depth #2

Merged
Arye Ramaty merged 11 commits from David-Haver/blender-old:davidhaver-WIP-realize-depth into WIP-realize-depth 2024-03-21 14:55:23 +01:00
Conversation 0 Commits 11 Files Changed 5 +325 -265
David-Haver commented 2024-03-21 00:28:08 +01:00
Contributor
Copy Link

more changes
I think it is ready for code reveiw

more changes I think it is ready for code reveiw
David-Haver added 8 commits 2024-03-21 00:28:17 +01:00
David-Haver added 1 commit 2024-03-21 12:38:18 +01:00
Arye Ramaty requested changes 2024-03-21 12:49:04 +01:00
Arye Ramaty left a comment
Owner
Copy Link

I didn't have time to review all the changes in the 'realize_instance' file, but that's it for now.

I didn't have time to review all the changes in the 'realize_instance' file, but that's it for now.
source/blender/geometry/intern/join_geometries.cc
@ -74,8 +74,8 @@ static void fill_new_attribute(const Span<const GeometryComponent *> src_compone
}
Arye Ramaty commented 2024-03-21 11:58:25 +01:00
Owner
Copy Link

I would rather not make clean-up or style changes to files I didn't modify in the PR. It just makes it a little bit harder to see what has changed.

I would rather not make clean-up or style changes to files I didn't modify in the PR. It just makes it a little bit harder to see what has changed.
David-Haver commented 2024-03-21 13:05:59 +01:00
Author
Contributor
Copy Link

the changes were done because you moved the declaration to the .hh file so it needed to follow the convention of public functions

the changes were done because you moved the declaration to the .hh file so it needed to follow the convention of public functions
Arye Ramaty commented 2024-03-21 14:52:59 +01:00
Owner
Copy Link

OK

OK
aryeramaty marked this conversation as resolved
source/blender/geometry/intern/realize_instances.cc
@ -2,23 +2,14 @@
*
Arye Ramaty commented 2024-03-21 12:15:01 +01:00
Owner
Copy Link

As a general rule, I'll try to minimize the length of the PR, aiming to stay as close as possible to origin/main. I believe things like 'include clean-up' can wait for a later PR.

As a general rule, I'll try to minimize the length of the PR, aiming to stay as close as possible to origin/main. I believe things like 'include clean-up' can wait for a later PR.
David-Haver commented 2024-03-21 13:09:02 +01:00
Author
Contributor
Copy Link

The changes were done mainly to the code you add and to thing that changed because of it.
New included made some other unnecessary.

The changes were done mainly to the code you add and to thing that changed because of it. New included made some other unnecessary.
Arye Ramaty commented 2024-03-21 14:52:45 +01:00
Owner
Copy Link

Fair enough

Fair enough
aryeramaty marked this conversation as resolved
source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc Outdated
@ -44,3 +45,3 @@
static auto depth_override = mf::build::SI2_SO<int, bool, int>(
"depth_override",
[](int value, bool realize) { return realize ? -1 : std::max(value, 0); },
[](int value, bool realize) { return realize ? -1 : value; },
Arye Ramaty commented 2024-03-21 12:02:09 +01:00
Owner
Copy Link

The use of std::max was intentional. We aim to prevent users from entering -1 into the field. Instead, we provide the 'realize all' option for that purpose.
This was suggested in the chat

The use of std::max was intentional. We aim to prevent users from entering -1 into the field. Instead, we provide the 'realize all' option for that purpose. This was suggested in the chat
David-Haver commented 2024-03-21 13:14:50 +01:00
Author
Contributor
Copy Link

From my understanding the users can't enter -1 because of .min(0) in line 27.
If it don't work like this I will change this back.

From my understanding the users can't enter -1 because of .min(0) in line 27. If it don't work like this I will change this back.
Arye Ramaty commented 2024-03-21 14:32:45 +01:00
Owner
Copy Link

The user can enter -1; the .min(0) serves as a 'soft limit,' implying that the user can still set the value lower than 0 by either typing a specific value (instead of scrubbing) or connecting the socket to a link.

The user can enter -1; the .min(0) serves as a 'soft limit,' implying that the user can still set the value lower than 0 by either typing a specific value (instead of scrubbing) or connecting the socket to a link.
David-Haver commented 2024-03-21 14:54:36 +01:00
Author
Contributor
Copy Link

ok returned it

ok returned it
aryeramaty marked this conversation as resolved
source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc Outdated
@ -66,2 +67,2 @@
const VArray<int> depths = evaluator.get_evaluated<int>(0);
const IndexMask selection = evaluator.get_evaluated_selection_as_mask();
geometry::SpecificInstancesChoice chosen_instances;
Arye Ramaty commented 2024-03-21 12:12:45 +01:00
Owner
Copy Link

This is fine, but I'm not sure about the name 'SpecificInstancesChoice'. I think something like 'SelectionAndDepthOption' may be clearer. Alternatively, 'VariedDepthOption' could also work.

This is fine, but I'm not sure about the name 'SpecificInstancesChoice'. I think something like 'SelectionAndDepthOption' may be clearer. Alternatively, 'VariedDepthOption' could also work.
David-Haver commented 2024-03-21 13:16:46 +01:00
Author
Contributor
Copy Link

I couldn't think of a better name VariedDepthOption sound great, I will change it to that.

I couldn't think of a better name VariedDepthOption sound great, I will change it to that.
David-Haver marked this conversation as resolved
David-Haver added 1 commit 2024-03-21 13:23:02 +01:00
David-Haver added 1 commit 2024-03-21 14:53:46 +01:00
Arye Ramaty merged commit 4370ae85cf into WIP-realize-depth 2024-03-21 14:55:23 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Arye Ramaty
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: aryeramaty/blender#2
No description provided.
Delete Branch "David-Haver/blender-old:davidhaver-WIP-realize-depth"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?