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

feat(spine): Adds a Spine Package #9

Merged
merged 16 commits into from
Apr 22, 2024
Merged

feat(spine): Adds a Spine Package #9

merged 16 commits into from
Apr 22, 2024

Conversation

GoodBoyDigital
Copy link

  • fixes up some tests
  • adds spineAtlasCompress
  • adds spineAtlasMipmap
  • add skipChildren to Asset which allows for skipping of assets in the manifest
  • strip atlas textures from Pixi Manifest (note this can be extracted to a parser in a future PR)

GoodBoyDigital and others added 11 commits April 19, 2024 21:05
- fixed json and test
- fixed texture packer still creating individual images
- ensure output is removed if the cache key changes
- fix compressed pngs not working
- remove spineAtlasMipmap from mipmap-compress
- fix png compression
- add spine package
@GoodBoyDigital GoodBoyDigital requested a review from Zyie April 22, 2024 11:14
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Your PR title doesn't match the required format. The title should be in the conventional commit (https://www.conventionalcommits.org/en/v1.0.0-beta.4/) format. e.g.

chore(plugin-name): add pr title workflow

@Zyie Zyie changed the title Adds a Spine Package to Asset PAck feat(spine): Adds a Spine Package Apr 22, 2024
packages/core/src/Asset.ts Outdated Show resolved Hide resolved
@@ -100,15 +133,19 @@ function collectAssets(
{
bundleAssets.push({
alias: getShortNames(stripTags(path.relative(entryPath, `${asset.path}-${pageIndex}`)), options),
src: pages.map((finalAsset) => path.relative(outputPath, finalAsset.path))
src: pages
Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't seem like we are adding data anymore?

we used to add all the tags to the manifest

@@ -20,7 +20,7 @@ export async function compressSharp(
compressed.push({
format: '.png',
resolution: image.resolution,
sharpImage: sharpImage.clone(),
sharpImage: sharpImage.clone().png({ ...options.png as PngOptions, force: true }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems weird, why is it being compressed multiple times?

const defaultOptions = {
..._options,
tags: {
tps: 'tps',
Copy link
Collaborator

@Zyie Zyie Apr 22, 2024

Choose a reason for hiding this comment

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

this doesn't seem right, why would spine need a tps tag?
also you are missing the nc tag definition

import type { CompressOptions } from '@play-co/assetpack-plugin-mipmap-compress';
import { AtlasView } from './AtlasView';

export type SpineAtlasCompressOptions = PluginOptions<'tps' | 'nc'> & CompressOptions;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here


export type SpineAtlasCompressOptions = PluginOptions<'tps' | 'nc'> & CompressOptions;

export function spineAtlasCompress(_options?: SpineAtlasCompressOptions): AssetPipe<SpineAtlasCompressOptions>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make sure variables are not _

expect(rawAtlasWebpHalf.includes('[email protected]')).toBeTruthy();
});

// it('should correctly create files when Mipmap and CacheBuster are used', async () =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason why these have been left?

@@ -1,12 +1,13 @@
import type { Asset, AssetPipe, PluginOptions } from '@play-co/assetpack-core';
import { checkExt, createNewAssetAt, swapExt } from '@play-co/assetpack-core';
import fs from 'fs-extra';
import type { CompressOptions } from '@play-co/assetpack-plugin-mipmap-compress';
Copy link
Collaborator

Choose a reason for hiding this comment

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

you've added a dependency here, but not added it to the package.json


export function texturePackerCompress(_options?: TexturePackerCompressOptions): AssetPipe<TexturePackerCompressOptions>
{
const defaultOptions = {
..._options,
tags: {
tps: 'tps',
Copy link
Collaborator

Choose a reason for hiding this comment

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

no definition for the nc tag

@GoodBoyDigital GoodBoyDigital requested a review from Zyie April 22, 2024 12:44
@Zyie Zyie merged commit 469fb68 into main Apr 22, 2024
2 of 3 checks passed
@Zyie Zyie deleted the feature/spine branch April 22, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants