Jump to content

#1325 Replace ecvt() in FCollada with something else

crazy_Baboon

By crazy_Baboon,
in Bug reports

 Share

Recommended Posts

crazy_Baboon Discens

crazy_Baboon

Posted
Posted (edited)

Hi Folks,

Following https://trac.wildfiregames.com/ticket/1325#no1, which has not been updated in 6 years.

I was trying to build 0ad on my FreeBSD desktop.

I have successfully built FCollada using the musl libc implementation of ecvt(). Since musl libc is MIT licensed, would the following code be considered to be included on 0ad, as part of FUStringBuilder.hpp: 

char *ecvt_musl(double x, int n, int *dp, int *sign)
{
	static char buf[16];
	char tmp[32];
	int i, j;

	if (n-1U > 15) n = 15;
	sprintf(tmp, "%.*e", n-1, x);
	i = *sign = (tmp[0]=='-');
	for (j=0; tmp[i]!='e'; j+=(tmp[i++]!='.'))
		buf[j] = tmp[i];
	buf[j] = 0;
	*dp = atoi(tmp+i+1)+1;

	return buf;
}



template <class Char, class FloatType>
void FloatToString(FloatType f, Char* sz)
{
	Char* buffer = sz + 1;
	static const int digitCount = 6;
	int decimal, sign;

	// ecvt rounds the string for us: http://www.datafocus.com/docs/man3/ecvt.3.asp
	char* end = ecvt_musl(f, digitCount, &decimal, &sign);

	if (sign != 0) (*buffer++) = '-';
	int count = digitCount;
	if (decimal > digitCount)
	{
		// We use the scientific notation: P.MeX
		(*buffer++) = (*end++); // P is one character.
		(*buffer++) = '.';

		// Mantissa (cleaned for zeroes)
		for (--count; count > 0; --count) if (end[count - 1] != '0') break;
		for (int i = 0; i < count; ++i) (*buffer++) = (*end++);
		if (buffer[-1] == '.') --buffer;

		// Exponent
		(*buffer++) = 'e';
		uint32 exponent = decimal - 1; // X
		if (exponent >= 10) (*buffer++) = (Char) ('0' + (exponent / 10));
		(*buffer++) = (Char) ('0' + (exponent % 10));
		(*buffer) = 0;
		return;
	}
	else if (decimal > 0)
	{
		// Simple number: A.B
		for (int i = 0; i < decimal; ++i) (*buffer++) = (*end++);
		if (decimal < digitCount) (*buffer++) = '.';
		count = digitCount - decimal;
	}
	else if (decimal < -digitCount)
	{
		// What case is this?
		decimal = count = 0;
	}
	else if (decimal < 0 || (decimal == 0 && *end != '0'))
	{
		// Tiny number: 0.Me-X
		(*buffer++) = '0'; (*buffer++) = '.';
		for (int i = 0; i < -decimal; ++i) (*buffer++) = '0';
		count = digitCount + decimal;
	}
	for (; count > 0; --count) if (end[count - 1] != '0') break;
	for (int i = 0; i < count; ++i) (*buffer++) = (*end++);
	if (decimal == 0 && count == 0) (*buffer++) = '0';
	if (buffer[-1] == '.') --buffer;
	(*buffer) = 0;
}

I basically replaced the call to ecvt() by ecvt_musl().

 

What do you guys think?

 

Cheers

Edited by crazy_Baboon
  • Like 1
Link to comment
Share on other sites
Stan` Consul

Stan`

Posted
Posted

Hey there thanks for your interest in ticket #1325.

The solution you propose seems nice however we need to test it on Mac OS and Windows as well if it's supposed to be a cross platform solution. If not we could add your patch to the build system for free bsd only. I Believe we already do that already. Is that version suffering from the same issues than the previous one detailed in the ticket ?

Tagging @historic_bruno in case he reads the forum

EDIT1: For patch submissions the best place is code.wildfiregames.com :)

EDIT2: I can probably test on windows if it's not too complicated to get that lib. Mac Os might be a bit more troublesome, though.

Link to comment
Share on other sites
Stan` Consul

Stan`

Posted
Posted
Just now, crazy_Baboon said:

Thanks Stan' for the quick reply. It works successfully on Linux Fedora.

https://code.wildfiregames.com/P186

https://trac.wildfiregames.com/ticket/1325#comment:10

I am sorry I am a complete noob with Phabricator and SVN!

 

Is git easier for you ? We provide a GitHub mirror https://github.com/0ad/0ad

Can you submit a differential ? See https://trac.wildfiregames.com/wiki/SubmittingPatches

Thanks for keeping it up :)

Let me know if I can help with anything.

  • Thanks 1
Link to comment
Share on other sites
crazy_Baboon Discens

crazy_Baboon

Posted
  • Author
Posted (edited)

Thanks!

I'll try to get arc to work on my desktop FreeBSD this weekend.

 

Meanwhile here's the diff file (in case it's useful):

diff --git a/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp b/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
index 272ab632e3..522be026e3 100644
--- a/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
+++ b/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp
@@ -18,14 +18,28 @@
 #include <float.h>
 #endif
 
-#ifdef WIN32
-#define ecvt _ecvt
-#endif // WIN32
-
 #ifndef SAFE_DELETE_ARRAY
 #define SAFE_DELETE_ARRAY(ptr) if (ptr != NULL) { delete [] ptr; ptr = NULL; }
 #endif
 
+// The implementation of ecvt was taken from musl libc (musl/src/stdlib/ecvt.c)
+char *ecvt_musl(double x, int n, int *dp, int *sign)
+{
+    static char buf[16];
+    char tmp[32];
+    int i, j;
+
+    if (n-1U > 15) n = 15;
+    sprintf(tmp, "%.*e", n-1, x);
+    i = *sign = (tmp[0]=='-');
+    for (j=0; tmp!='e'; j+=(tmp[i++]!='.'))
+        buf[j] = tmp;
+    buf[j] = 0;
+    *dp = atoi(tmp+i+1)+1;
+
+    return buf;
+}
+
 template <class Char, class FloatType>
 void FloatToString(FloatType f, Char* sz)
 {
@@ -34,7 +48,7 @@ void FloatToString(FloatType f, Char* sz)
     int decimal, sign;
 
     // ecvt rounds the string for us: http://www.datafocus.com/docs/man3/ecvt.3.asp
-    char* end = ecvt(f, digitCount, &decimal, &sign);
+    char* end = ecvt_musl(f, digitCount, &decimal, &sign);
 
     if (sign != 0) (*buffer++) = '-';
     int count = digitCount;

 

 

 

 

 

Edited by crazy_Baboon
Link to comment
Share on other sites
crazy_Baboon Discens

crazy_Baboon

Posted
  • Author
Posted

Yep, 

so I have downloaded the diff file (D2399.diff) and saved it on my 0ad git base folder.

when I do in the base git directory: 

git apply -p . D2399.diff

I get: 

error: patch failed: libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp:18
error: libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp: patch does not apply

Does anyone know how to fix this?
 

git version 2.24.0
FreeBSD 12.1-RELEASE

Link to comment
Share on other sites
crazy_Baboon Discens

crazy_Baboon

Posted
  • Author
Posted (edited)

ok. On a brand new 0ad git clone folder:

I just tried git apply -p . D1592.diff --reject : 

D1592.diff:46: trailing whitespace.
 
D1592.diff:88: trailing whitespace.
 
D1592.diff:94: trailing whitespace.
 
D1592.diff:127: trailing whitespace.
 
D1592.diff:136: trailing whitespace.
 
Checking patch libraries/source/spidermonkey/FixpsutilFreeBSD.diff...
Applied patch libraries/source/spidermonkey/FixpsutilFreeBSD.diff cleanly.
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

ok, this seems to have worked. Now moving on to

git apply -p . D1593.diff --reject : 

Checking patch source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp...
error: while searching for:

void AtObj::add(const char* key, const wxString& value)
{
	add(key, value.wc_str());
}

void AtObj::add(const char* key, const wchar_t* value)

error: patch failed: source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp:164
error: while searching for:

void AtObj::set(const char* key, const wxString& value)
{
	set(key, value.wc_str());
}

void AtObj::set(const char* key, const wchar_t* value)

error: patch failed: source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp:187
Checking patch source/tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp...
error: while searching for:
	else
	{
		wxString filePath = GetSelectedFilePath();
		AtlasMessage::qVFSFileExists qry(filePath.wc_str());
		qry.Post();
		if (!filePath.IsEmpty() && qry.exists)
		{
			AtlasMessage::qVFSFileRealPath qry(filePath.wc_str());
			qry.Post();
			wxDynamicCast(FindWindow(ID_MapDialogFilename), wxTextCtrl)->ChangeValue(*qry.realPath);
		}

error: patch failed: source/tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp:166
Checking patch source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp...
Applying patch source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp with 2 rejects...
Rejected hunk #1.
Rejected hunk #2.
Applying patch source/tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Applied patch source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp cleanly.

@Stan` Do you know why these git conflicts are happening? What can be done to fix them?

Edited by crazy_Baboon
Link to comment
Share on other sites
crazy_Baboon Discens

crazy_Baboon

Posted
  • Author
Posted

@Stan`

Hum... I thought so, but similar errors happen to my patch: and that one is quite recent... 

git apply -p . D2399.diff --reject

  

Checking patch libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp...
error: while searching for:
#include <float.h>
#endif

#ifdef WIN32
#define ecvt _ecvt
#endif // WIN32

#ifndef SAFE_DELETE_ARRAY
#define SAFE_DELETE_ARRAY(ptr) if (ptr != NULL) { delete [] ptr; ptr = NULL; }
#endif

template <class Char, class FloatType>
void FloatToString(FloatType f, Char* sz)
{

error: patch failed: libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp:18
error: while searching for:
    int decimal, sign;

    // ecvt rounds the string for us: http://www.datafocus.com/docs/man3/ecvt.3.asp
    char* end = ecvt(f, digitCount, &decimal, &sign);

    if (sign != 0) (*buffer++) = '-';
    int count = digitCount;

error: patch failed: libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp:34
Applying patch libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp with 2 rejects...
Rejected hunk #1.
Rejected hunk #2.

Is this process of "applying patches" any friendlier in SVN?

 

 

 

 

 

Link to comment
Share on other sites
Stan` Consul

Stan`

Posted
Posted
1 hour ago, crazy_Baboon said:

Is this process of "applying patches" any friendlier in SVN?

On Windows maybe but I don't think it is on Linux.

Can you try using Arcanist if it's not too hard for you ? I believe the problem is that this file might have CRLF Line endings which causes a tantrum on Linux...

 

 

 

 

 

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...