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

Do not include construction images in extrude/lathe/revolve/helix #1420

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

ruevs
Copy link
Member

@ruevs ruevs commented Nov 15, 2023

To avoid unnecessary/annoying copies of images added in 2d sketches if they are marked "construction" they will not be copied when creating solids.

Fixes: #1418

To avoid unnecessary/annoying copies of images added in 2d sketches if they
are marked "construction" they will not be copied when creating solids.

Fixes: solvespace#1418
@phkahler
Copy link
Member

@ruevs Would it be too messy to add a separate case for IMAGE ? I'm not sure how much of the default stuff applies to them.

@phkahler
Copy link
Member

phkahler commented Nov 15, 2023

@ruevs Another thing... I tried 3.1 on Windows and can't select an image to make it construction. There's also no way to visually tell if an image is construction or not.

Oh I see in the video. You have to select the image in the text window and it will highlight. Then you can right-click to select construction. I suppose this is deliberate because people want to trace over an image and don't want it keep highlighting and getting selected...

@ruevs
Copy link
Member Author

ruevs commented Nov 15, 2023

@ruevs Another thing... I tried 3.1 on Windows and can't select an image to make it construction. There's also no way to visually tell if an image is construction or not.

Oh I see in the video. You have to select the image in the text window and it will highlight. Then you can right-click to select construction. I suppose this is deliberate because people want to trace over an image and don't want it keep highlighting and getting selected...

For cross-reference - here is the video in question #1418 (comment)

@ruevs Would it be too messy to add a separate case for IMAGE ? I'm not sure how much of the default stuff applies to them.

Well I thought of this, but the "default stuff" is what copies the corner points of the image entity in the 3d groups. And I thought those could always be useful and do not get in the way much. It can easily be changed of course.

@jkrei0
Copy link

jkrei0 commented Nov 15, 2023

I'm not sure what use case there is for keeping images in the extrude by default (or at all); it seems more reasonable to me to have them hidden by default. And if you really want the images, you can always add them back in a new sketch.

I don't dislike the idea of having images be togglable, but it would make more sense to have it either be a checkbox in the text window (not sure how feasable that is to implement), or to have the default be to not copy images.

This approach (of making "construction" images not be copied) feels a bit unintuitive and hidden, especially since (if I understand correctly) you have to select images from the text window when the sketch group is active and then right-click in the graphics area, and since in all other cases, construction entities are copied.

@phkahler
Copy link
Member

@ruevs I was going to suggest having images be construction entities by default, which is backwards compatible and does what we want by default with your fix. But I'm with @jkrei0 in thinking this is a huge hack that changes the meaning of "construction" just for images, while also being nearly impossible to find.

I'm not sure how many people use images, but I'm guessing far less want the image to get copied. They can manually add extra copies if they want.

@ruevs
Copy link
Member Author

ruevs commented Nov 17, 2023

This approach (of making "construction" images not be copied) feels a bit unintuitive and hidden, especially since (if I understand correctly) you have to select images from the text window when the sketch group is active and then right-click in the graphics area, and since in all other cases, construction entities are copied.

When you are in a 2d (sketch) group the face selection is disabled image by default. If you enable it images can be selected. Here is the code:

if(e.type == Entity::Type::IMAGE && !showFaces) continue;
)

After selecting them pressing G toggles construction - just like for any other entity. No need to right-click.

I did it through the text window in the video just to make it explicitly obvious that you have to select the original image in the sketch and not one of its copies in the extruded object. For the same reason I showed that when the extrusion is "two sided" you can select the original image from the 3d (extrude) group. And for the same reason I used the right-click menu.

What is more while placing an image (still dragging its corners around for the first time) one can press G to make it construction to begin with. An of course it is trivial to make images construction by default - but it will change the existing default behaviour and I did not want to do that on my own initiative but I can if you want. As @phkahler already noticed the test case will need to be adjusted - no problem in principle.

As for:

changes the meaning of "construction" just for images, while also being nearly impossible to find.

Yes it does - but images are "special anyway" - and while logically (in the code) it changes the meaning of construction in my opinion for the user it does not. Construction is "geometry that does not produce solids" from the users point of view and images are always that, just that when we mark them "construction" they produce even less "stuff".

And discoverability - well not everything has to be blatantly obvious. There is a price to pay (in cluttered user interfaces) for "discoverability". Whoever has gotten to the point of using images to "trace over" and is annoyed by their replication (if we do not make construction the default) - should read a bit of manual :-)

So finally - should I add a commit to make images construction by default?
It is just adding

                    r->construction = true;

after

r->file = pending.filename;

@phkahler
Copy link
Member

Whoever has gotten to the point of using images to "trace over" and is annoyed by their replication (if we do not make construction the default) - should read a bit of manual :-)

My understanding is that tracing over is THE reason for images in sketches. The replication must be something people have been annoyed with for years but never complained about. My opinion is that they should be flagged as construction by default regardless, so other parts of the code don't have to filter them now or in the future. But I also think the copies should be unconditionally squashed rather than only suppressed for construction images.

BTW, faces and images should not be selectable in a 2d mode because they'd keep lighting up which would be really annoying. I only brought up the fact that they don't highlight and can't be right-click toggled to construction to point out the difficulty of using their construction flag, not to suggest we change it.

So yes to making them construction by default, and no to letting them get copied at all.

With this change and the previous commit images will not be included in
3d groups by default.
@phkahler phkahler merged commit 5edc1ec into solvespace:master Nov 22, 2023
3 of 4 checks passed
Copy link
Member

@phkahler phkahler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough. I'm still not sure we should ever copy images but you'll have to try real hard to do so with this ;-)

@ruevs ruevs deleted the DoNotCopyConstructionImages branch November 22, 2023 22:21
@ruevs ruevs added the UI label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images from a sketch are "extruded" and get in the way
3 participants