GUI: Change in the BDF fonts scaling code #4867

NischayDiwan · 2023-04-03T11:12:33Z

The primary change is to fix or improve the scaling of BDF fonts
Now uses the same approach as in MacFonts scaling
Bugs - there are still some problems when testing in capital letters
Note: the change in graphics.cpp for the testbed engine now calls the scale font function.

sev-

This code is not correct. You are generating dstGray but then do not use it at all.

sev- · 2023-04-03T15:53:30Z

graphics/fonts/bdf.cpp

 }

+#if DRAWDEBUG
+bool dododo11;


Why is this sitting in a common space and not in the method?

Also, if you want to keep it, please give it a meaningful name

I have copied this as it is from the MacFont Code, it basically allows the debug to run in the Magnify gray function as well.
I will remove that.

sev- · 2023-04-03T15:54:22Z

graphics/fonts/bdf.cpp

-	data.defaultBox.xOffset = (int)(roundf((float)src->_data.defaultBox.xOffset * scale));
-	data.defaultBox.yOffset = (int)(roundf((float)src->_data.defaultBox.yOffset * scale));
-	data.ascent = (int)roundf(((float)src->_data.ascent * scale));
+	data.maxAdvance = (int)(((float)src->_data.maxAdvance * scale));


Are you sure you do not want to round but use the floor()?

Oh, I missed that, will fix it.

sev- · 2023-04-03T15:55:27Z

graphics/fonts/bdf.cpp

-		if (src->_data.bitmaps[i]) {
-			const int bytes = ((box.width + 7) / 8) * box.height; // Dimensions have been already corrected
-			bitmaps[i] = new byte[bytes];
+		// int hs = srcBox.height;


Why is this commented out and not removed?

Yes I will fix this.

graphics/fonts/bdf.cpp

sev- · 2023-04-03T16:00:37Z

graphics/fonts/bdf.cpp

+					}
+#endif
+					if (*grayPtr > grayLevel)
+						*dst = 1;


Could you please refactor this magnification method, stick it into Common::Font (base method) and then reuse it in both MacFont and here?

I don't understand, what to change here please clarify

Refactor the magnification of a srcSurf via dstGray into a separate reusable method.

sev- · 2023-04-03T16:01:09Z

graphics/fonts/bdf.cpp

+					debugN("\n");
+#endif	
+			}
+			const int bytes = dstPitch*h;


This does not conform to our code formatting conventions.

I will fix that.

graphics/fonts/bdf.cpp

sev-

Too many strange decisions. And still not following our code formatting guidelines

sev- · 2023-04-06T18:02:42Z

graphics/fonts/bdf.cpp

+
+#if DRAWDEBUG
+		int ccc = 'a';
+		dododo11 = i == ccc;


Why is it still here?

Yes fixed that

sev- · 2023-04-06T18:02:56Z

graphics/font.cpp

+	grayScaleMap = (int *)malloc(grayScaleMapSize * sizeof(int));
+}
+
+Surface *Font::scaleSingleGlyph(int width, int height, int grayLevel, int chr, int xOffset, int srcheight, int srcwidth, float scale){


Bad code formatting

I think it is the function line, please check I have changed it

sev- · 2023-04-06T18:03:12Z

graphics/font.cpp

+	return scaleSurface;
+}
+
+void Font::deinitializeScaling(){


Bad code formatting

yes same fixed it

sev- · 2023-04-06T18:04:22Z

graphics/fonts/macfont.cpp

-				PixelFormat::createFormatCLUT8());
-	int dstGraySize = newSize * 20 * newSize;
-	int *dstGray = (int *)malloc(dstGraySize * sizeof(int));
+	BdfFont* srcCopy = new BdfFont(src->_data, DisposeAfterUse::YES);


Nad code formatting. And WHY BdfFont????

Yes removed that, check the new commit

sev- · 2023-04-06T18:05:57Z

graphics/fonts/macfont.cpp

+	srcCopy->initializeScaling(src->getFontSize(), newSize);
+
+	Graphics::Surface tmpSurf;
+	// srcSurf.create(MAX(src->getFontSize() * 2, newSize * 2), MAX(src->getFontSize() * 2, newSize * 2),


Why is this still commented out instead of being removed?

sev- · 2023-04-06T18:06:18Z

graphics/fonts/macfont.cpp

+	// int dstGraySize = newSize * 20 * newSize;
+	// int *dstGray = (int *)malloc(dstGraySize * sizeof(int));

 	tmpSurf.create(MAX(src->getFontSize() * 2, newSize * 2), MAX(src->getFontSize() * 2 + 2, newSize * 2 + 2),


Why is this still here?

fixed the comments, tmpSurf is not used in any of those 3 functions, but outside though so I have kept it as it is

Then please move its creation closer to the usage.

yes, I have fixed that but It has to be declare in the outside the for loop.

graphics/fonts/macfont.cpp

sev-

More changes are still needed.

sev- · 2023-04-06T20:14:04Z

graphics/font.h

 class Font {
 public:
-	Font() {}
+	Font() {}; 


What is the intent of this change?

I added the semicolon earlier by mistake, it was not in original files

graphics/font.h

sev- · 2023-04-06T20:15:06Z

graphics/font.h

+	/**
+	 * Initializes the scalesurface for scaling and the grayScaleMap for the given @p srcSize and @p newSize.
+	 *
+	 * @param srcSize


Here you have to describe the parameters, not just enumerate them

sev- · 2023-04-06T20:18:33Z

graphics/font.h

+	 *
+	 * @param width     The width of the bouding box.
+	 * @param height    The height of the bounding box.
+	 * @param grayLevel The gray level.


What is "gray level"? you need to describe it here.

sev- · 2023-04-06T20:18:44Z

graphics/font.h

+	/**
+	 * Scales the single gylph at @p chr the given the @p scale and @p resGray and returns the scaled surface with updated @p resGray
+	 *
+	 * @param width     The width of the bouding box.


bounding box of what?

graphics/fonts/bdf.cpp

sev- · 2023-04-08T11:36:13Z

graphics/fonts/bdf.cpp

-
+#if DRAWDEBUG
+			if (i == ccc) {
+				warning("BdfFont::scaleFont(): %d %d", data.height,box.height);


Do you need this as a warning? And if you do, the code formatting is off.

yes that is not required, removed it now

sev- · 2023-04-08T11:38:55Z

graphics/fonts/bdf.cpp

 		return nullptr;
 	}

+	BdfFont *srcCopy = new BdfFont(src->_data, DisposeAfterUse::YES);


Why are you creating a whole copy of the entire font? Please, do not do it.

sev- · 2023-04-08T11:39:30Z

graphics/fonts/bdf.cpp

 	}

+	BdfFont *srcCopy = new BdfFont(src->_data, DisposeAfterUse::YES);
+	srcCopy->initializeScaling(src->getFontSize(), newSize);


initializeScaling() should be called for this copy of the object, not creating another one pointlessly.

sev- · 2023-04-08T11:39:57Z

graphics/fonts/macfont.cpp

+	// int dstGraySize = newSize * 20 * newSize;
+	// int *dstGray = (int *)malloc(dstGraySize * sizeof(int));

 	tmpSurf.create(MAX(src->getFontSize() * 2, newSize * 2), MAX(src->getFontSize() * 2 + 2, newSize * 2 + 2),


Then please move its creation closer to the usage.

sev-

Still, a lot of problems, starting from basics that are code formatting, ending with memory leaks, possible memory override and in general, oddities.

As we discussed on Discord, I recommend allocating dstGray() in the caller code, e.g. in the scaleFont() calls and pass it to the scaleSingleGlyph(). This way you just avoid any modifications to the class state. Instead, you decided to go your own way, but the implementation is very problematic. I outlined it in the comments.

sev- · 2023-04-10T21:52:36Z

engines/testbed/graphics.cpp

 	for (int i = origFont->getFontHeight(); i <= 20; i++) {

-		//const Graphics::BdfFont *font = Graphics::BdfFont::scaleFont(origFont, i);
+		// const Graphics::BdfFont *font = Graphics::BdfFont::scaleFont(origFont, i);


What is the purpose of this change?

sev- · 2023-04-10T21:54:14Z

graphics/fonts/bdf.cpp

 	: _data(data), _dispose(dispose) {
 }

+// BdfFont::BdfFont(DisposeAfterUse::Flag dispose)


What is the purpose of this code?

removed the comments

sev- · 2023-04-10T21:55:53Z

graphics/fonts/bdf.cpp

 }

+void BdfFont::assignFontData(const BdfFontData &data) {
+	_data = data;


And what is happening with the previous thing which was in _data?

sev- · 2023-04-10T21:56:28Z

graphics/fonts/bdf.cpp


 	float scale = (float)newSize / (float)src->getFontSize();

+	BdfFont *scaledFont = new BdfFont(src->_data, DisposeAfterUse::YES);


Why are you passing src->_data here when later you just overwrite it? Please, avoid it.

sev- · 2023-04-10T21:57:26Z

graphics/fonts/bdf.cpp

 	return new BdfFont(data, DisposeAfterUse::YES);
 }

+void BdfFont::assignFontData(const BdfFontData &data) {


What about the _dispose flag? Also, in accordance with other similar methods in other classes, it is better name it setData()

I initialize the Font using the constructor, and this method only changes the data. I will rename it to setData. _dispose flag is handled by the constructor used at the beginning

sev- · 2023-04-10T21:57:58Z

graphics/fonts/bdf.h

 class BdfFont : public Font {
 public:
 	BdfFont(const BdfFontData &data, DisposeAfterUse::Flag dispose);
+	// BdfFont(DisposeAfterUse::Flag dispose);


Why is it here?

removed this

sev- · 2023-04-10T22:02:58Z

graphics/fonts/bdf.cpp

+			const int bytes = dstPitch * box.height;
+#if DRAWDEBUG
+			if (i == ccc) {
+				int *dstGray = *dstGrayPointer;


What is the purpose of this declaration which immediately goes out of scope?

dstGray is being used in the debug part of code, so needed declaration. It was getting its value from dstGrayPointer defined above, now changed it and removed dstGrayPointer, as it was redundant.

If you want to declare it, then you declare it incorrectly. Try to compile with DRAWDEBUG on. It will not even compile!

sev- · 2023-04-10T22:04:11Z

graphics/fonts/bdf.cpp

-			const int bytes = ((box.width + 7) / 8) * box.height; // Dimensions have been already corrected
+			int *dstGrayPointer = 0;
+			Graphics::Surface *srcSurf = scaledFont->scaleSingleGlyph(box.width, box.height, grayLevel, i + src->_data.firstCharacter, 
+																  	  box.xOffset, src->_data.height, srcBox.width, scale, &dstGrayPointer);


Okay, now you allocate dstGrayPointer in scaleSingleGlyph(). Where do you free this memory?

it is happening in the deconstructor

sorry I missunderstood, I have replace it to dstGra, which is required for debuging

graphics/fonts/macfont.cpp

sev- · 2023-04-14T15:08:56Z

graphics/fonts/bdf.cpp

 				byte b = 0;
-
-				for (int x = 0; x < box.width; x++) {
+				for (int x = 0; x < box.width; x++,srcd++) {


This does not conform to our code formatting guidelines.

- The refactoring code for MacFont that magnifies the font on a surface and then uses a grayscalemap to refactor the scaling, is pushed back into a resuable method in the base font class. - surface magnigy funtion transfer and scaleSingleGlyph method add to the font class.

- The primary change is to fix or improve the scaling of BDF fonts - Now uses the same approach as in MacFonts scaling that is creating another surface and refactor it with a grayscalemap - Bugs - there are still some problems when testing in capital letters

sev- · 2023-04-16T19:13:45Z

Thank you!

NischayDiwan force-pushed the master branch from 71ee73a to a563c3b Compare April 3, 2023 12:09

sev- requested changes Apr 3, 2023

View reviewed changes

sev- requested changes Apr 6, 2023

View reviewed changes

sev- requested changes Apr 8, 2023

View reviewed changes

sev- requested changes Apr 10, 2023

View reviewed changes

NischayDiwan force-pushed the master branch 2 times, most recently from 2c4dff0 to 9672dad Compare April 11, 2023 17:00

sev- requested changes Apr 14, 2023

View reviewed changes

NischayDiwan force-pushed the master branch from b215888 to 5efa7ca Compare April 16, 2023 14:34

sev- merged commit aaa32cf into scummvm:master Apr 16, 2023


		float scale = (float)newSize / (float)src->getFontSize();

		BdfFont *scaledFont = new BdfFont(src->_data, DisposeAfterUse::YES);

Uh oh!

GUI: Change in the BDF fonts scaling code #4867

GUI: Change in the BDF fonts scaling code #4867

Uh oh!

Conversation

NischayDiwan commented Apr 3, 2023

Uh oh!

sev- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sev- Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sev- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sev- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sev- left a comment •

edited

Loading

sev- Apr 3, 2023 •

edited

Loading