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(): Missing features from fabric 5 and types exports #8954

Merged
merged 14 commits into from
May 24, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- feat(): Export setFilterBackend and port the texture filtering option from fabric 5 [#8954](https://github.com/fabricjs/fabric.js/pull/8954)
- chore(): swap commonly used string with constants [#8933](https://github.com/fabricjs/fabric.js/pull/8933)
- chore(TS): Add more text types [#8941](https://github.com/fabricjs/fabric.js/pull/8941)
- ci(): fix changelog action race condition [#8949](https://github.com/fabricjs/fabric.js/pull/8949)
Expand Down
9 changes: 9 additions & 0 deletions rollup.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ const plugins = [
* @param {*} warn
*/
function onwarn(warning, warn) {
// we error at any warning.
// we allow-list the errors we understand are not harmful
if (
warning.code === 'PLUGIN_WARNING' &&
!warning.message.includes('sourcemap')
) {
console.error(chalk.redBright(warning.message));
throw Object.assign(new Error(), warning);
}
Copy link
Member Author

@asturur asturur May 24, 2023

Choose a reason for hiding this comment

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

@ShaMan123 let's have the build error on default if types are broken.
We can comment this locally when we need to work in a broken state with watchers, but i feel better if the pipeline is strict about this, otherwise we found ourselves with unexpected cleanups that we can't see when we code review the PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree
That sourcemap warning must be resoled as well

if (warning.code === 'CIRCULAR_DEPENDENCY') {
console.error(chalk.redBright(warning.message));
throw Object.assign(new Error(), warning);
Expand Down
9 changes: 4 additions & 5 deletions src/canvas/StaticCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1319,17 +1319,16 @@ export class StaticCanvas<
if (!isTextObject(obj)) {
return;
}
let fontFamily = obj.fontFamily;
const { styles, fontFamily } = obj;
if (fontList[fontFamily] || !fontPaths[fontFamily]) {
return;
}
fontList[fontFamily] = true;
if (!obj.styles) {
if (!styles) {
return;
}
Object.values(obj.styles).forEach((styleRow) => {
Object.values(styleRow).forEach((textCharStyle) => {
fontFamily = textCharStyle.fontFamily;
Object.values(styles).forEach((styleRow) => {
Object.values(styleRow).forEach(({ fontFamily = '' }) => {
Comment on lines +1322 to +1331
Copy link
Contributor

Choose a reason for hiding this comment

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

I have touched this in my cleanup #8952

Copy link
Member Author

Choose a reason for hiding this comment

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

i think the only thing i really changed is es6 leverage

if (!fontList[fontFamily] && fontPaths[fontFamily]) {
fontList[fontFamily] = true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/filters/BlendImage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export class BlendImage extends BaseFilter {
applyToWebGL(options: TWebGLPipelineState) {
const gl = options.context,
texture = this.createTexture(options.filterBackend, this.image);
this.bindAdditionalTexture(gl, texture, gl.TEXTURE1);
this.bindAdditionalTexture(gl, texture!, gl.TEXTURE1);
super.applyToWebGL(options);
this.unbindAdditionalTexture(gl, gl.TEXTURE1);
}
Expand Down
4 changes: 4 additions & 0 deletions src/filters/FilterBackend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,7 @@ export function getFilterBackend(strict = true): FilterBackend {
}
return filterBackend;
}

export function setFilterBackend(backend: FilterBackend) {
filterBackend = backend;
}
63 changes: 41 additions & 22 deletions src/filters/WebGLFilterBackend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ export class WebGLFilterBackend {
width,
height,
!cachedTexture ? source : undefined
),
)!,
passes: filters.length,
webgl: true,
aPosition: this.aPosition,
Expand Down Expand Up @@ -243,42 +243,55 @@ export class WebGLFilterBackend {
* Accepts specific dimensions to initialize the texture to or a source image.
*
* @param {WebGLRenderingContext} gl The GL context to use for creating the texture.
* @param {Number} width The width to initialize the texture at.
* @param {Number} height The height to initialize the texture.
* @param {HTMLImageElement|HTMLCanvasElement} textureImageSource A source for the texture data.
* @param {number} width The width to initialize the texture at.
* @param {number} height The height to initialize the texture.
* @param {TexImageSource} textureImageSource A source for the texture data.
* @param {number} filter gl.NEAREST default or gl.LINEAR filters for the texture.
* This filter is very useful for LUTs filters. If you need interpolation use gl.LINEAR
* @returns {WebGLTexture}
*/
createTexture(
gl: WebGLRenderingContext,
width: number,
height: number,
textureImageSource?: TexImageSource
textureImageSource?: TexImageSource,
filter = gl.NEAREST
) {
const {
TEXTURE_2D,
RGBA,
UNSIGNED_BYTE,
CLAMP_TO_EDGE,
TEXTURE_MAG_FILTER,
TEXTURE_MIN_FILTER,
TEXTURE_WRAP_S,
TEXTURE_WRAP_T,
} = gl;
const texture = gl.createTexture();
gl.bindTexture(gl.TEXTURE_2D, texture);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE);
gl.bindTexture(TEXTURE_2D, texture);
gl.texParameteri(TEXTURE_2D, TEXTURE_MAG_FILTER, filter);
gl.texParameteri(TEXTURE_2D, TEXTURE_MIN_FILTER, filter);
gl.texParameteri(TEXTURE_2D, TEXTURE_WRAP_S, CLAMP_TO_EDGE);
gl.texParameteri(TEXTURE_2D, TEXTURE_WRAP_T, CLAMP_TO_EDGE);
if (textureImageSource) {
gl.texImage2D(
gl.TEXTURE_2D,
TEXTURE_2D,
0,
gl.RGBA,
gl.RGBA,
gl.UNSIGNED_BYTE,
RGBA,
RGBA,
UNSIGNED_BYTE,
textureImageSource
);
} else {
gl.texImage2D(
gl.TEXTURE_2D,
TEXTURE_2D,
0,
gl.RGBA,
RGBA,
width,
height,
0,
gl.RGBA,
gl.UNSIGNED_BYTE,
RGBA,
UNSIGNED_BYTE,
null
);
}
Expand All @@ -294,17 +307,23 @@ export class WebGLFilterBackend {
* @param {HTMLImageElement|HTMLCanvasElement} textureImageSource A source to use to create the
* texture cache entry if one does not already exist.
*/
getCachedTexture(uniqueId: string, textureImageSource: TexImageSource) {
if (this.textureCache[uniqueId]) {
return this.textureCache[uniqueId];
getCachedTexture(
uniqueId: string,
textureImageSource: TexImageSource
): WebGLTexture | null {
const { textureCache } = this;
if (textureCache[uniqueId]) {
return textureCache[uniqueId];
} else {
const texture = this.createTexture(
this.gl,
textureImageSource.width,
textureImageSource.height,
textureImageSource
);
this.textureCache[uniqueId] = texture;
if (texture) {
textureCache[uniqueId] = texture;
}
return texture;
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/filters/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
export * as filters from './filters';

export { getFilterBackend, initFilterBackend } from './FilterBackend';
export {
getFilterBackend,
initFilterBackend,
setFilterBackend,
} from './FilterBackend';
export { Canvas2dFilterBackend } from './Canvas2dFilterBackend';
export { WebGLFilterBackend } from './WebGLFilterBackend';
export { isWebGLPipelineState } from './utils';
Expand Down
2 changes: 1 addition & 1 deletion src/filters/typedefs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Canvas2dFilterBackend } from './Canvas2dFilterBackend';

export type TProgramCache = any;

export type TTextureCache = any;
export type TTextureCache = Record<string, WebGLTexture>;

export type TPipelineResources = {
blendImage?: HTMLCanvasElement;
Expand Down
42 changes: 13 additions & 29 deletions src/shapes/Text/StyledText.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,10 @@ import type {
} from '../Object/types';
import { FabricObject } from '../Object/FabricObject';
import { styleProperties } from './constants';
import type { StylePropertiesType } from './constants';
import type { Text } from './Text';

export type TextStyleDeclaration = Partial<
Pick<
Text,
| 'fill'
| 'stroke'
| 'strokeWidth'
| 'fontSize'
| 'fontFamily'
| 'fontWeight'
| 'fontStyle'
| 'textBackgroundColor'
| 'deltaY'
| 'overline'
| 'underline'
| 'linethrough'
>
>;
export type TextStyleDeclaration = Partial<Pick<Text, StylePropertiesType>>;

export type TextStyle = {
[line: number | string]: { [char: number | string]: TextStyleDeclaration };
Expand All @@ -38,7 +23,7 @@ export abstract class StyledText<
declare abstract styles: TextStyle;
protected declare abstract _textLines: string[][];
protected declare _forceClearCache: boolean;
static _styleProperties = styleProperties;
static _styleProperties: Readonly<StylePropertiesType[]> = styleProperties;
abstract get2DCursorLocation(
selectionStart: number,
skipWrapping?: boolean
Expand Down Expand Up @@ -78,8 +63,8 @@ export abstract class StyledText<
* @param {Number} lineIndex to check the style on
* @return {Boolean}
*/
styleHas(property: string, lineIndex?: number): boolean {
if (!this.styles || !property || property === '') {
styleHas(property: keyof TextStyleDeclaration, lineIndex?: number): boolean {
if (!this.styles) {
return false;
}
if (typeof lineIndex !== 'undefined' && !this.styles[lineIndex]) {
Expand Down Expand Up @@ -111,8 +96,8 @@ export abstract class StyledText<
*
* @param {string} property The property to compare between characters and text.
*/
cleanStyle(property: string) {
if (!this.styles || !property || property === '') {
cleanStyle(property: keyof TextStyleDeclaration) {
if (!this.styles) {
return false;
}
const obj = this.styles;
Expand All @@ -124,12 +109,9 @@ export abstract class StyledText<
for (const p1 in obj) {
letterCount = 0;
for (const p2 in obj[p1]) {
const styleObject = obj[p1][p2],
const styleObject = obj[p1][p2] || {},
// TODO: this shouldn't be necessary anymore with modern browsers
stylePropertyHasBeenSet = Object.prototype.hasOwnProperty.call(
styleObject,
property
);
stylePropertyHasBeenSet = styleObject[property] !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

#8953 => but the rest are typeof meanwhile.


stylesCount++;

Expand Down Expand Up @@ -164,6 +146,7 @@ export abstract class StyledText<
graphemeCount += this._textLines[i].length;
}
if (allStyleObjectPropertiesMatch && stylesCount === graphemeCount) {
// @ts-expect-error conspiracy theory of TS
Copy link
Contributor

Choose a reason for hiding this comment

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

And don't know if to laugh or cry
These annoying errors

this[property as keyof this] = stylePropertyValue;
this.removeStyle(property);
}
Expand All @@ -176,8 +159,8 @@ export abstract class StyledText<
*
* @param {String} props The property to remove from character styles.
*/
removeStyle(property: string) {
if (!this.styles || !property || property === '') {
removeStyle(property: keyof TextStyleDeclaration) {
if (!this.styles) {
return;
}
const obj = this.styles;
Expand Down Expand Up @@ -289,6 +272,7 @@ export abstract class StyledText<
styleProps = (this.constructor as typeof StyledText)._styleProperties;
for (let i = 0; i < styleProps.length; i++) {
const prop = styleProps[i];
// @ts-expect-error TS complains even when we serve everything on a silver plate.
styleObject[prop] =
typeof style[prop] === 'undefined'
? this[prop as keyof this]
Expand Down
16 changes: 15 additions & 1 deletion src/shapes/Text/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,21 @@ export const additionalProps = [
'direction',
] as const;

export const styleProperties = [
export type StylePropertiesType =
| 'fill'
| 'stroke'
| 'strokeWidth'
| 'fontSize'
| 'fontFamily'
| 'fontWeight'
| 'fontStyle'
| 'textBackgroundColor'
| 'deltaY'
| 'overline'
| 'underline'
| 'linethrough';

export const styleProperties: Readonly<StylePropertiesType[]> = [
...fontProperties,
...textDecorationProperties,
'stroke',
Expand Down
5 changes: 1 addition & 4 deletions src/util/misc/textStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,7 @@ export const stylesFromArray = (
return cloneDeep(styles);
}
const textLines = text.split('\n'),
stylesObject = {} as Record<
string | number,
Record<string | number, Record<string, string>>
>;
stylesObject: TextStyle = {};
let charIndex = -1,
styleIndex = 0;
//loop through each textLine
Expand Down