search

4ian / GDevelop

:video_game: Open-source, cross-platform game engine designed to be used by everyone.
https://gdevelop.io
Other
6.56k stars 722 forks source link

Bitmap Text #1894

Closed Bouh closed 3 years ago

Bouh commented 3 years ago

In WIP

This bitmapText can't allow to edit the styles like the other text.. There is no styles in _pixiObject :/ i'am not able to edit, color, fontFamily. Only once instance is show at the time.

4ian commented 3 years ago

Very interesting 👀 Please ping me with a comment on the PR/code if you need something.

Bouh commented 3 years ago

I'm beginning to understand how bitmap works. I have all the instances visible now! I'm going step by step ^_^

Silver-Streak commented 3 years ago

@Bouh As always (especially with this), if you need testing let me know.

Bouh commented 3 years ago

There is a strange error when i start the preview. i = "remove"

image

Also the font are visible and can be updated in the IDE, except the default Arial police is not rendering, i wondering why because the fontFamily is set on Arial.

4ian commented 3 years ago

There is a strange error when i start the preview.

data.char seems completely broken, it's an array but it has lot of weird stuff. Can you try to get step by step in the debugger in BitmapFont.from to see when things are starting to be fishy? (typically when does "remove" gets added to this array? Make no sense!).

Can be because of some invalid data passed to Pixi.js at some point.

i wondering why because the fontFamily is set on Arial.

This may not be supported because the font is not really loaded. Is the bug only happening for Arial?

Bouh commented 3 years ago

Found it !

In gd.js the prototype remove and createFrom appear in the array, when i comment these two proto, i can see the bitmapText with the font in the preview :)

image

4ian commented 3 years ago

Oh! Very interesting. That's why they say "don't mess with the builtins prototypes" 😬

Though I wonder if Pixi.js needs a fix in the way it iterates on the array? I see a lot of for in in https://pixijs.download/dev/docs/packages_text-bitmap_src_BitmapFont.js.html#line190 which could be replaced by for of.

See the difference between for of and for in for arrays: https://stackoverflow.com/questions/29285897/what-is-the-difference-between-for-in-and-for-of-statements-in-jav#:~:text=Difference%20for..in%20and%20for,property%20keys%20of%20an%20object&text=Examples%20of%20iterable%20objects%20are%20arrays%2C%20strings%2C%20and%20NodeLists.

But to be honest, we should avoid messing with the Array prototype. createFrom should be fairly easy to rewrite as a function taking two arguments. I think the same for remove (search for remove( in the codebase to see where we use it), though I've not done the search myself yet. (all of this should be handled in a different PR).

(Might be also a good time to rework how we use remove, because it's super inefficient).

Bouh commented 3 years ago

There is a fix in progress in pixi repo :) https://github.com/pixijs/pixi.js/pull/6790 I'll use this for finish this PR.

Also all styles aren't available for now. https://github.com/pixijs/pixi.js/issues/6789

4ian commented 3 years ago

There is a fix in progress in pixi repo :)

Great! 👍👍

Also all styles aren't available for now.

These limitations are ok I would say!

Bouh commented 3 years ago

Notes for me for the next commits:

One question why use dirty on the pixi object for color, fontSize, fontFamilly, and not on the opacity and rawText ?

IDE:

Game preview:

Wait update/bugfixes from Pixi and other:

4ian commented 3 years ago

One question why use dirty on the pixi object for color, fontSize, fontFamilly, and not on the opacity and rawText ?

Usually in this case dirty is used to say to the PIXI object "we modified something that is inside the style, so we use dirty to let you know that we did this without you knowing". In fact, most changes that you do on a PIXI object will turns into setting a "dirty" flag:

This being said, all of this is usually deduced from just trying the object :) Set the color, see if it renders properly the new color. Yes? Cool, no need to set dirty! No? Then maybe you misused the object (check the PIXI doc) and/or maybe you need to set the dirty flag.

Bouh commented 3 years ago

This being said, all of this is usually deduced from just trying the object :) Set the color, see if it renders properly the new color. Yes? Cool, no need to set dirty! No? Then maybe you misused the object (check the PIXI doc) and/or maybe you need to set the dirty flag.

Yes i test everything the documentation and the styles aren't all yet complete, i compare with Bitmap feature in the playground of Pixi. I take my notes here and i'll report what i've seen to Pixi team.

Silver-Streak commented 3 years ago

It's worthwhile mentioning that the Pixi team's big "Pixi Next (V6)" post, they list unifying the BitmapText and main text styling methods. but it sounds like that's a year or so away.

Edit: Link here for reference. https://github.com/pixijs/pixi.js/issues/6595

4ian commented 3 years ago

Just want to make sure again that's it's not big deal if not all styling can be applied to BitmapText. If something is not supported in Pixi, we're not going to add it now :) Let's just make this object to work as best as possible using all the Pixi existing capabilities then ship it :)

Bouh commented 3 years ago

Almost image

4ian commented 3 years ago

There are some files in this PR that have been modified in master, surely because it was based on a PR that was force pushed. Maybe reopening the PR would fix the way GitHub display the changes?

Bouh commented 3 years ago

Oof I need cherry pick all my commits? Maybe I can just revert the bad change ? Btw this is not yet finish there is lot of offset on the text positions. But I can't work on it until 16august.

Le jeu. 13 août 2020 à 22:57, Florian Rival notifications@github.com a écrit :

There are some files in this PR that have been modified in master, surely because it was based on a PR that was force pushed. Maybe reopening the PR would fix the way GitHub display the changes?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1894#issuecomment-673706250, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMX4DTI4O5C23ABP6P6L5LSARHSHANCNFSM4PH6CGAQ .

Bouh commented 3 years ago

Or can i merge master here ?

Le jeu. 13 août 2020 à 23:13, Aurélien Vivet bouhvivez@gmail.com a écrit :

Oof I need cherry pick all my commits? Maybe I can just revert the bad change ? Btw this is not yet finish there is lot of offset on the text positions. But I can't work on it until 16august.

Le jeu. 13 août 2020 à 22:57, Florian Rival notifications@github.com a écrit :

There are some files in this PR that have been modified in master, surely because it was based on a PR that was force pushed. Maybe reopening the PR would fix the way GitHub display the changes?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1894#issuecomment-673706250, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMX4DTI4O5C23ABP6P6L5LSARHSHANCNFSM4PH6CGAQ .

arthuro555 commented 3 years ago

that was force pushed

Changing history affects only people who had an older version of the tree. They only break stuff on the branch it was force pushed to if someone had the previous without an history rewrite. In fact for every commit in master, by squashing commits, you rewrite history as one commit in the branch that gets merged into master, that is still not counting as an history rewrite on master.

Or can i merge master here ?

Try git rebase ;)

4ian commented 3 years ago

Arthuro555 Yep I know, Bouh merged (https://github.com/4ian/GDevelop/commit/49efc49b55a5df02ecb5720741db7357ed1fb3a9) another PR that I then force-pushed/squashed. I could have just merged but forgot it was used here.

Anyway, merge master into this PR and it should be good. If GitHub still show unrelated changes, close and re-open the PR (it's a bug that can happen where existing changes are still shown in the PR)

Bouh commented 3 years ago

Well the conflicts are solved, i can run the app.

I don't know well what's wrong with the wrap mode and the width, but there is something wrong somewhere i can't found where.

The white sprite on right should fit the blue selection from the scene editor. image

I've used the dev tools, the width of the pixiObject is over 900px, but nothing seem setup an value like this.

Silver-Streak commented 3 years ago

@Bouh Just checking in on this. Are you still making progress?

Bouh commented 3 years ago

I waiting the first review from 4ian, he have planned to do this and I'll rewrite the actions/conditions.

Le sam. 22 août 2020 à 04:14, Silver-Streak notifications@github.com a écrit :

@Bouh https://github.com/Bouh Just checking in on this. Are you still making progress?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1894#issuecomment-678580260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMX4DXNPINBZGL4G4JWSG3SB4SXNANCNFSM4PH6CGAQ .

4ian commented 3 years ago

The white sprite on right should fit the blue selection from the scene editor.

I've looked quickly at the code, I'm not sure exactly what is happening but can you log the content of the object? To check wordWrap and wrappingWidth values.

Also something to add to the TODOs for this PR before merging: we'll make a manager class or at least an object holding the bitmap fonts that are generated, and surely we'll ask the objects to register their font slug (and unregister it when they are destroyed, or when the font slug is changed, for example if you change the font or the size, or anything that requires to make again the font).

Why doing that? Because it will allow us to destroy from memory a bitmap font that is not used anymore :) Better, we'll be able to make a slightly more advanced logic, which would allow for example to keep ~10 bitmap font in memory (so that if you delete all objects using a font, the font stays in memory, unless there are more than 10 different fonts). This would allow to avoid reloading always the same font (for example when changing scenes, objects from the previous scene are destroyed) and still avoid exploding the memory usage in a game where a lot of objects are created and destroyed with a different/random font/size/color/etc.

Bouh commented 3 years ago

I can't figure out why the dimensions aren't correct, it drives me crazy. The values in the editor are correct, and accessible in the renderer for the preview, but the rendering is not the same...

It would be great to have a single rendering method for an object. And if necessary overwrite the default method to have a different rendering in the IDE. I always feel like I'm doing double work when I do an extension.

Silver-Streak commented 3 years ago

@Bouh The editor isn't PixiJS itself, but the preview is, right?

Does that mean it's something with how the PixiJS calls are being passed for the objects? If so, maybe it would be worthwhile to ask on the PixiJS Github if you and 4ian aren't able to crack the issue.

Bouh commented 3 years ago

The preview and the IDE use Pixi in the same version 5.3.3 I think the problem is an missunderstood from me in the renderer for the preview. Pixi team will not help me with this, it work fine in the IDE, so isn't a issue from Pixi.

4ian commented 3 years ago

I'll take a look this week-end :) @Bouh can you prepare an example project file that I can open and quickly see the issue? I guess that's only what I need, in addition to pulling your branch.

Bouh commented 3 years ago

Sure, thx you! BitmapTextObject_project.zip

4ian commented 3 years ago

@Bouh I'm working on some changes, I think I found the issue of the positioning:

I'm also doing lots of simplification because it's useless to compare some values, as Pixi is already marking the object "dirty" by itself when you changes something (and does not mark it dirty when you set a value to the same value as before).

EDIT: I'm also changing "fontFamily", because it's a bad name chosen for BBText. It should be "fontResourceName". EDIT2: I'm also:

4ian commented 3 years ago

@Bouh here you go! :)

The rendering should be consistent between the IDE/game, and most stuff should have been fixed. I've not tested all the actions/conditions though, so maybe something it out of place but should be fairly easy to fix now that the main stuff are here, done and working.

Here is the updated project you sent me: BitmapTextObject_project 2.zip

As I renamed a field (fontColor), there was the need to go in the JSON and change the fields too. Previous objects already saved won't work, you'll have to recreate them if you used in other tests/examples.

Still to do:

Bouh commented 3 years ago

I'll read what has change and see what are the next things to do with this new clean base! Thank for the help guys !

What you mean by the width seems unreliable? This need to be reported to the PIXI team? Every object have an width and height, I think textWidth should be the widthof the pixi object. I mean get the width of an PIXI.Sprite return the width of the texture. It should be the same idea for the bitmapText, get the width should return the textWidth.

The rendering should be consistent between the IDE/game, and most stuff should have been fixed.

Oh gosh thank you!

I've not tested all the actions/conditions though, so maybe something it out of place but should be fairly easy to fix now that the main stuff are here, done and working.

Well now i can retake over and test everything and finish. I was confused, I relied too much on BBtext.

Bouh commented 3 years ago

The color format for Tweens behavior is fixed. The interpolation work fine with the Tween behavior, and not with the action setColor related to BitmapText x) It's same for other actions from bitmapText... In the eventsheet the actions from BitmapText object have zero influence on the games, the value is always the value from the object in the IDE. I looked to see if there was a syntax error somewhere but no!

Oh and it didn't work either when I pulled the changes made this weekend.

4ian commented 3 years ago

Ah I remember starting to look at this and I think there is no code generation made for the BitmapText actions.. might be a misconfiguration somewhere! I'll take another look if needed.

Speaking of tween behavior, I think we should maybe NOT support the change by color (for this we can rename get/setColor to get/setFontColor). Why so? Because the BitmapText color is stored inside the generated texture for the font. Usually, we tween values that are super cheap to change (a position in space, a color which is usually just a tint applied to the object).

Here if we change the color 60 times per second, we'll create 60 copies of the bitmap font in memory 😱 Even if we create a smart bitmap font manager that unload the fonts, it's a lot of resource intensive operations.

Instead, I recommend we have two things:

The tint is just some color added on top of the rendered text, so should be very cheap :) Not exactly the same, but it's like the "global color" of a sprite.

Bouh commented 3 years ago

It could be possible to use the tint as the color maybe ? We setup the default fontColor on white, the tint in black, and the properties and the instructions can be called 60times/s which any colors.

Edit: we can't the result are not correct, the font seem bolder. image

Peoples are already mess when he use the action for change the global color of the sprite, because it's the tint not the color, user think the result will be the color choosen by himself. In fact it's a blend of the current texture color and the tint from the global color action.

Global Color may be should rename to tint. Because Add a red tint/global color on an black object will return an black object.

Silver-Streak commented 3 years ago

It could be possible to use the tint as the color maybe ? We setup the default fontColor on white, the tint in black, and the properties and the instructions can be called 60times/s which any colors.

@Bouh Just to chime in there, this seems less like bold, and more like the text Outline has been enabled somehow, and then tinted along with the main font. Bold would normally be much thicker than that.

(I could be wrong)

4ian commented 3 years ago

Agree that we should rename "global color" to "tint color" - "global color" is a terrible name :p I recommend we go ahead with both for the object: a font color as we have now (get/setFontColor) and a tint (internally using get/setColor, to be supported by the tweens). To reduce confusion, maybe let's not have the "tint" as a property in the editor, but just have an action to "set the object tint color" (much like sprite tint can be just modified using the events)?

Bouh commented 3 years ago

The bitmapFontManager is now in the bitmapText extension, and the cache is functionnal!

Here how the font are stored: image

10 font in the cache to unloaded is a lot. 5 max is enough, and i think it's still too much because each font generate an texture of 512 wide by default (for now up to 1024 in this PR, but i need to check if an issue was fixed in PIXI before reduce to the default value, 1024 is a workaround for missing characters in the generation of the font).

About the alphabets, currently the numeric, punctuation and [A-Z], [a-z] characters are supported. Here the texture font generated by PIXI, found with spectorJS téléchargement

My question is, which other alphabets we should include? According to wikipedia, there is 143 859 unicode characters, and we can't support all of them (write 143 859 characters in a charset table with an size of 12px is impossible). If the characters are not in the bitmapFont generated then the text on the screen will be empty, here is my dilemma.

I was wondering why not use the text to display and put it in the charset from the font to generate.

Silver-Streak commented 3 years ago

@Bouh and I were talking about it on the Discord, but I did some checking and most other engines do it this way:

Include ASCII characters by default:

space ! “ # $ % & ‘ ( ) * + , - . / 0 1 2 3 4 5 6 7 8 9 : ; < = > ? @ A B C D E F G H I J K L M N O P Q R S T U V W X Y Z [ \ ] ^ _ ` a b c d e f g h i j k l m n o p q r s t u v w x y z { | } ~

Then allow devs to specify additional characters to include.

A few engines specifically look at the provided font file and parse all available characters, but that seems unneeded.

Having the devs provide what additional characters they want ensures that you're not generating unneeded characters.

4ian commented 3 years ago

10 font in the cache to unloaded is a lot

Happy to reduce this to 5 if you think it's better memory wise.

The bitmapFontManager is now in the bitmapText extension, and the cache is functionnal!

Nice! Good job :)

Then allow devs to specify additional characters to include.

I think it's the best workaround. Support ASCII by default, and allow dev to specify, using a property, a few other additional characters. @bouh if you do this, remember that this is part of the font definition, so this must be included in the font name (the string finishing by -bitmapFont) in the cache, along with the size, color and name. Indeed, much like changing the color, changing these additional characters should lead to a new font in memory.

Bouh commented 3 years ago

I seeking for why the events didn't work with the bitmapText extension, this issue is arrived after you have made your commits, so i've tried to compare the commits, but i see nothing special.

I've added breakpoints on functions, but none was called. So i've looked on the generated code, and the functions are missing in the generated code.

I've tried to merge current master and this PR, but the functions are still missing from the generated code. Should i add the functions for cocos renderer? Even empty?

4ian commented 3 years ago

Hypothesis:

All the parameters are declared as taking a BitmapTextObject: 

.addParameter('object', _('Bitmap text'), 'BitmapTextObject', false)

while the object is declared as a BitmapText. So code generation will discard everything as the object type is not the proper one.

Change all parameters to have the object type to be BitmapText instead of BitmapTextObject and hopefully it will work :)

Bouh commented 3 years ago

@Silver-Streak here an early build. Let me know if you encounter any problems.

I still have to make a field to support special characters. Then it'll be ready for review and merge.

4ian commented 3 years ago

Sounds good! 👍

Silver-Streak commented 3 years ago

@Bouh @4ian Unfortunately, I'm the bearer of bad news. (as normal 😢 )

image

Top text: Bitmap Font Object Middle Text: Normal Text object Bottom Text: Sprite Object from a PNG made in Gimp with the exact same font at the exact same size.

Game Resolution: 640x360 At start of scene, resize (Scale) to 1920x1080.

Bitmap font has different spacing, which isn't a huge deal as so does the GIMP PNG. Unfortunately, it's very apparent that something is still happening with scaling on Bitmap font, that doesn't happen with the sprite object. (Edit: To be clear, I mean that it's becoming very blurry unlike the Sprite object at the bottom. )

As a super wild guess: It almost seems like the same scaling as the normal Text Object is getting applied to Bitmap Font, rather than treating it as a bitmap/sprite/etc.

Bouh commented 3 years ago

Hi! Can you provide the project? Have you disabled the antialiasing before? (even if i think the anwser is yes)

The bitmapFont can be regenerated when you call an action for the font size, the fill color, and the font family. Can you try to change the color when the resolution is downscaled? How the font look like in the IDE when you zoom in/out?

I wonder where the problem is, it's like going back to the starting point. It's the bitmap object, the way to render from GDevelop, the batch render queue in pixi or the positioning of objects that are not rounded. I don't know, i wonder the problem is how gdevelop compute the dimension of canvas, because as far i know the issue isn't in other app, see ctjs, c2/3.

I'll try to start from this sandbox on jsfiddle and try to rescale the canvas.

Silver-Streak commented 3 years ago

@Bouh You got it, here's a zip of the project.

Preview the scene and press P (for 720p) or L (for 1080p) or M (for Fullscreen) to see the impact.

I've also set the font to change tint to white (255/255/255) and resize font size whenever it changes resolution as a test, neither impacts the blurriness. :(

FontScaleTest2.zip

Edit: I should also mention that after saving and reloading the project in the IDE the font appearance is DRASTICALLY different than in the preview:

IDE: image

Updated Preview: image

4ian commented 3 years ago

If this is still blurry with the Bitmap Text object, this means the texture where was rendered the text was blurry. Are you sure you rendered the text with the exact font size the font was made for?

If yes, this might be a bug report for Pixi.js ("we try to render a bitmap font with a font, but this font is blurry from the beginning") and also another argument for adding the capability to load a bitmap font from a texture (I'm not super familiar, but I think "BMFont" is a well known/used format )

4ian commented 3 years ago

Also be sure that the text position is rounded.

I should also mention that after saving and reloading the project in the IDE the font appearance is DRASTICALLY different than in the preview:

Looks like a bug, is this just for the bitmap text or also the existing text object?

Silver-Streak commented 3 years ago

If this is still blurry with the Bitmap Text object, this means the texture where was rendered the text was blurry. Are you sure you rendered the text with the exact font size the font was made for?

If yes, this might be a bug report for Pixi.js ("we try to render a bitmap font with a font, but this font is blurry from the beginning") and also another argument for adding the capability to load a bitmap font from a texture (I'm not super familiar, but I think "BMFont" is a well known/used format )

@4ian Bitmap Text/Font Object is made with monogram.ttf at font size 16. Text Obect is made with monogram.ttf at font size 16. PNG Sprite object is made in GIMP with monogram.ttf at font size 16.

(Source here: https://datagoblin.itch.io/monogram and 16 point is a recommended size, but additionally, as it renders correctly in the PNG even after scaling, this would make me think that the font itself is fine)

Also be sure that the text position is rounded.

Gave it a shot by rounding the text positions, does not change anything that I can see (all text positions are whole integers already, but wanted to check to make sure) image image

I should also mention that after saving and reloading the project in the IDE the font appearance is DRASTICALLY different than in the preview:

Looks like a bug, is this just for the bitmap text or also the existing text object? Both Bitmap Text and Existing text, but both of them behave differently. (you can see in the above screenshot from my prior post from the IDE)

This is something that used to happen with BBText as well, but was fixed a while back it seems. (There is an example of BBText in the example project that displays correctly in the preview and in the IDE)

IDE: image

Preview: image

(Still blurry, obviously, but positioning/spacing still correct)

Additionally, the boundary boxes in the IDE are correct. just the rendering of the text are wrong.

(Highlighting BMText object boundary box, you can see text goes way outside of it in the IDE)

image