Open Sample Errors on Car Wheel Sample

[jgomez720]

Description

Using "Open Sample" on samples that use in() or mm() (or equivalent) causes errors. When I opened the Car Wheel sample, I got an error becuase the units were in mm(), but it is expecting inches. We need to also bring in the units along with the sample.

Version

v0.25.4

Recordings/Screenshots

[franknoirot]

Ah interesting! That makes sense, but it does complicate things. In the web we can just override the unit settings if you think that makes sense. In the desktop app, what should we do?

Like if I have a project with mm as my units for a project, I expect the user won't want to overwrite their project's units, right? Do we need a way to transform samples that use any unit function (like in() or mm()) to be the sample but using a different unit system? That sounds super hairy. Or do we only allow creating a new project if the file uses a unit function, because that means its units actually matter?

[franknoirot]

Or transform all raw literals in the KCL to use the unit function in the sample's setting if any unit function is used anywhere within that file? So because the car wheel sample uses an mm() we transform it to use in() on every literal value when we import?

[Irev-Dev]

how many uses of mm()/in() are there in the samples?
Do we just remove them all for samples?

[jgomez720]

When users import a sample, I'm fine with overwriting their units for that project, perhaps with some message "This project will switch your units to in" or something like that. If they are asking for a sample, I feel they should be okay with getting all of the sample (toml included)

[jgomez720]

There's only this one I believe @Irev-Dev. Might be one other, let me check.

There's two. Car Wheel and Wheel Rotor

[jtran]

Off of the top of my head, this makes me wish that every file could declare at the top which units it uses. This would allow you to import/copy a file from another project, and it would just work, regardless of what units you use. Mixing and matching wouldn't be a problem. Maybe it's not a best practice, but it would help ease friction with remixing.

(Sorry, this is in a world where there's actually multiple files...)

[Irev-Dev]

We did have the idea of putting project level settings in a special comment at the top of the files, (eventually including dependencies too). The idea being would help folks share minimal reproducible examples, so that sounds good.

[Irev-Dev]

But at the same time, if there's two usages, maybe we just remove those, instead of implementing a whole new feature to fix this problem.

[JordanNoone]

I'm liking @jtran's logic that the global units is a config setting in the KCL, that way it carries with the part.

[jessfraz]

adding my opinion from slack, ABSOLUTELY DO NOT ABUSE THE mm() in() functions where not necesary. We already ahve a way of knowing the units, its the project.toml, kcl-samples has that information, I say:

• read the project.toml for the projhect
• if users units differ prompt with a modal "This file is in inches cahnge project to use inches?" then they get the choice
• We need the same functionality for text to cad

[jessfraz]

in the future with assemblies we could do something like @jtran suggests, but also mixing units among lots of files seems like a bad idea and a source of pain in general, like imagine in our repo we had some analogue of this, it seems insane

[franknoirot]

I can read the project settings of the sample and provide warning of the sort @jessfraz is talking about easily.

[franknoirot]

Is it common for projects to be in one unit system by default, but then need subsystems or parts within it to use another unit system? Like is it common to have a project assembly in imperial that has one part in metric? Because that points toward needing some part- or file-specific unit definition.

[jessfraz]

i like @jtran suggestion but i dont want to add a second way of getting the units for files without really thinking it thru since we will be stuck with it forever, like 1. are we sure we even need that, 2. is there a better way, like lets actually put thought into it, not that @jtran didn't (maybe hes been sitting on this for days) i just dont want to do it as a fast fix response

[jessfraz]

because i imagine a scenario where users NEVER use the project settings and instead put inches at the top of every file, like oye...

[franknoirot]

Yeah 100% we should think about it for a while. We have a plan for a UX bug fix that requires no deep changes

[franknoirot]

For the purposes of this bug, we will overwrite the current user's project settings to match the sample if they don't match the current settings. We'll warn the user on the confirmation step so that they can cancel before they complete the import.

[jessfraz]

in most cases i think they'd overwrite, even better, when you load the thing that says "open in new project" tell them about the units then so they can open in a new prohect with those units.

[jtran]

Absolutely. Let's be careful and think through. I haven't done that yet. The reason it came to me is it's often nice to have everything self-contained in a single file. (There's even been proposals to put Cargo.toml contents in .rs files to make single-file scripts.) But maybe it doesn't apply to units. I don't want the format to encourage bad practices.

[franknoirot]

in most cases i think they'd overwrite, even better, when you load the thing that says "open in new project" tell them about the units then so they can open in a new prohect with those units.

I've broken that out into #3969