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

Export weldPrimitive #767

Merged
merged 3 commits into from
Jan 3, 2023
Merged

Conversation

marwie
Copy link
Contributor

@marwie marwie commented Jan 2, 2023

It would be nice to be able to re-use the weld and weldOnly methods for individual primitives without having to copy the code in our own extension/plugin.

I'm not sure if you'd want to export those methods and increasing the API surface. I would like to make a similar PR for the squoosh and/or toktx methods if you're generally open for that

@donmccurdy
Copy link
Owner

donmccurdy commented Jan 2, 2023

Thanks @marwie! I like the idea of exporting per-primitive functions. Per #565 (comment) I would vote to name the exported function something like weldPrimitive(document, prim, options), with tolerance=0 being equivalent to weldOnly.

I would like to make a similar PR for the squoosh and/or toktx methods if you're generally open for that...

Maybe! Currently most of the code in these functions has to do with choosing which textures to update. The code to compress textures (see #752) is actually pretty short:

const buffer = await encoder(texture.getImage())
  .toFormat('png')
  .toBuffer();
texture.setImage(BufferUtils.toView(buffer))

Especially since the current encoders (squoosh and toktx) have some major limitations, I'm a little worried that wrapping that short per-texture code might lead people to depend on those wrappers rather than just choosing the right encoder for their needs. Happy to discuss the options here though, perhaps in a new issue? Also see:

@donmccurdy donmccurdy added feature New enhancement or request package:functions labels Jan 2, 2023
@donmccurdy donmccurdy added this to the v2.5 milestone Jan 2, 2023
@marwie
Copy link
Contributor Author

marwie commented Jan 2, 2023

That's great. Applied your suggestion and moved the checks from the weld loop to the new weldPrimitive method

I understand that concern, especially since you're currently in the process of switching to sharp.
Maybe another alternative would be to be able to provide a callback via the options to modify the settings and/or processing altogether - the per slot / filter solution that e.g. toktx has right now doesn't really work for us very well since we want to allow users to control / override the compressions settings completely depending on the usecase.

I will open another issue for it later :)

@donmccurdy donmccurdy merged commit 60a895e into donmccurdy:main Jan 3, 2023
@donmccurdy
Copy link
Owner

Looks great, thanks!

@donmccurdy donmccurdy changed the title Export weldOnly and weldAndMerge Export weldPrimitive Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New enhancement or request package:functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants