@AmiDark
Don't get me wrong. it seems that very few peoples tried your previous free versions BUT all the donators knew that there were availables when you posted News on AmigaWorld or Amigans.
Nobody can blame you if your GK don't work (or if samples don't compile). I knew that I can't use them on my non-Warp3D system but I backed your bounty...

Last edited by zzd10h on 11-Jan-2015 at 06:40 AM.
Last edited by zzd10h on 11-Jan-2015 at 01:09 AM.

_________________
http://apps.amistore.net/zTools

@zzd10h :
I've just followed the procedure I give in the "howtocompile.txt" file, under my Windows XP using AmiDevCPP 0.98
And the library : libAmiDARK.a is correctly generated and compiled.

I then tried to compile a demonstration of the engine using AmiDevCPP with the freshly compiled libAmiDARK.A file and the technical demonstration compîled perfectly creating the executable file correctly.

But all of these were done under Windows not AmigaOS4.
As I have no more AmigaOS4 computer I cannot test.

I'll check my older versions because when I started the project, I compiled it directly on the Sam440EP... I shoud find my old custom makefile for AmigaOS4 and try to adapt it to the new version and changes.

@AmiDARK

"But all of these were done under Windows not AmigaOS4.
As I have no more AmigaOS4 computer I cannot test."

Yes, I know, when I tested your v0.9, 1 year ago, the samples compiled fine

Last edited by zzd10h on 11-Jan-2015 at 06:40 AM.
Last edited by zzd10h on 11-Jan-2015 at 01:31 AM.

_________________
http://apps.amistore.net/zTools

@Overflow
Quote:

Well, forums are bad at conveying "tone and attitude" when reading/making posts; so I generally just extract the useful information I get from a post, since I realise that I can easily misunderstand the "mood" of the poster. Im my honest opinion it does neither you nor Daytona675x any good to get "worked up" over the "tone" of a post. Both of your are skilled coders/developers: so its better for everyone you exchange information instead of focusing on "emotions". Ive had many forum exchanges over the years, and usually people get worked up over misunderstandings. Usually 1 minute on voice communication like Teamspeak, mumble etc has cleared up any "conflict". Bottom line; work together towards a common goal; improving the availability of development tools on AOS4?

I have to say, these are very wise words here.

@All : Project code updated this morning with few changes from the aros community.

Just as promised I'll invest some time to take a deeper look at the code.
It may be a matter of personal opinion if the code's quality is important to the bounty or not, since it simply is not mentioned.
I believe that people who paid for it should get (and most likely expect) at least some basic quality. At least core functionality should do what it is supposed to do and should be stable. I'd not consider the bounty fulfilled if even basic things are broken.

It's more or less the same thing as with other stuff: When I buy something I don't expect the item's description to contain a phrase like "it is not broken". If not said otherwise I simply expect it to work. At least I expect it not to blow up

If the author had clearly written "this is merely an early prototype" all would be fine.
But he did not. Instead he even now insists in the code being fine even if pointed to severe bugs.
Not even the bugs I already reported were fixed til now (for example the broken CalculateNormal function).

Of course it is not up to me to decide if the bounty can be considered fulfilled, that's up to the people who donated. My review on this "code" is to help them to better decide for themselves, to get them important information on the code / the code's quality they apparently won't get otherwise.
And of course to help improving this library.

Anyway, last not least because some forum users explicitely asked me for it, I'll try to review one source-area each day now
Today: Basic2D

The good news first:
I found no crash bugs at this area so far, yeeeha

But nevertheless there are bugs, of course. And quite some weirdness and inefficiencies.
In the following I only mention bugs and some selected weird / unelegant / whatever but uncritical issues that I find important to know to get an idea of the source-code's general quality. One more side-note: believe me, I'm still very diplomatic here... If I'd tell you my true uncensored opinion on each code-line...

BASIC2D_Constructor (incomplete)
Doesn't set all global Basic2D variables. TraceBasic2D got forgotten and will be in a random state.
Not too critical, since it will be set on startup by IMAGE_Constructor's call to INTERNAL_ResizeImageList.
This cross-init however already indicates spaghetti-code - and all the potential problems with that.

DEGetPixelsPitch (incomplete, useless)
"will return the pitch in bytes of the visual surface"
Nope, actually it just and always returns 0.

PasteB2DShapeElipse (buggy and low output quality)
B2D_Render.c, Line 151. What's that?
1) 6.5 is not 2*PI and it also isn't 2*PI+0.1 or whatever. As a result and in combination with the 0.1 stepping this shape will not look like it's supposed to.
2) always using a fixed step of 0.1 makes no sense (and 0.1 itself makes even less sense, something like (2*PI)/30 or so would have made some more). As a result the tessellation of small ellipses will be too high and that of large ellipses will be too low.

PasteB2DShapeElipse (efficiency)
B2D_Render.c, Line 152, 153:
You find such things all over the lib: useless casting from this to that and back and forth. Make up your mind first and chose the right data-types and matching functions for your task.
Line 154:
And why tell OGL to use doubles when that what you just calculated is just a float?

PasteB2DShapeLine (buggy)
B2D_Render.c, Line 172, 174:
Why are you suddenly rendering using integers?!
Let's see: shapes of type X are drawn using floats, shapes of type Y using doubles and shapes of type Z using ints. Now that's what I call consistency...
So if you want to draw a tangent to a circle for example, good luck. Depending on where you draw your stuff the line sometimes touches the circle correctly and sometimes it won't, thanks to the integer-drawing and loss of sub-pixel-precision here.

DECopyArea (useless code)
B2D_DrawShapes.c, Lines 152-155:
Symptomatic for huge parts of the lib: all those 4 checks are useless, in lines 140-151 he just made sure that those 4 checks always pass.

DECopyArea (undocumented behavior, most likely a bug)
B2D_DrawShapes.c, Lines 140-145:
If SourceX/Y are < 0 then they are set to 0 and the width/height is increased. So essentially the source-rectangle is silently moved into the visible area.
This MAY be desired. But it also may be that we got a bug here and the width/height should be decreased, so that the source-rectangle is silently clipped to the visible area.
Since the latter is also what is happening in lines 146-151 when it's about the right/bottom (clipped, not moved) I suppose this is a bug, resulting from a typo in lines 141 and 144 (- instead of +).
Since the "documentation" doesn't mention anything regarding negative coordinates and its consequences I can't tell for sure.

This also points to yet another problem:

The almost not existing documentation makes it impossible to tell if certain functions do what they should do - simply because what they shall do is not specified. So whenever a potential issue like the one above is found, you may always argue that this is desired behavior (even if it is not), and that the docs are incomplete only...

All this also makes this statement
Quote:

if developer uses the command in the range they are developed for) without crashing

somewhat laughable. Without documentation there is no definition of such sort of "range"...

Therefore reviewing this lib is a bit of a chicken / egg problem, because the first thing you get to hear is
Quote:

Documentation: Not yet available .. Dev status : Unfinished , mentioned in bounty.

which of course somewhat bites with his before-quoted statement...



The following are comments on some uncritical things I saw.

DERgbR
DERgbG
DERgbB
Trivial functions of this kind should be inline

DERGB (redundant)
DERGBR
DERGBG
DERGBB
Why have 100% identically-behaving wrappers that only differ in the function-names capital letters?
To add some extra-bloat to the lib? To artificially increase the number of functions? To make it appear bigger?
This makes no sense.

ShapeID (coding style)
Although this is hidden to the final user it would have been a good idea to use an enum/defines/const-identifiers for the ShapeIDs. Plain numbers were the worst design choice. Won't make it easier to fix/ enhance/ extend this lib. And why start with 0 or 1 if you can start counting with 3?!

Shape functions (missed opportunity)
Too bad that he missed the chance to add alpha-support. Yes, I know: compatibility with DarkBasic, yes, yes.
Only that it wouldn't have hurt.

DisplayBasic2DStruct (include / dependency / structure mess)
I guess this one deserves an own dedicated chapter in my upcoming reviews

Luckily the bugs I mentioned can be fixed in very short time. Let's see if that happens or if they remain unfixed like the ones I reported two days ago
See you tomorrow when the reviewing continues!

_________________
AmigaOS 4.1 FE (sam460ex Radeon 9200 / RadeonHD), MorphOS 3.8 (PowerMac G4 733MHz Radeon 9000), AROS (x86), A1200 (060 80MHz Indivision MK2), A500, A600, CDTV
Wings Remastered Development Diary

@Daytona675x
Will check all these today later.

DEGetPixelsPitch is here only for Compatibility with DarkBASIC Professional/DarkGDK.
I have found no way to get the information from OpenGL

Elipse drawing need "optimisation" but it work .... *Fixed Project*

CopyArea : I'll check this ... Theorically it shouldn't move the box is area contain parts out of display.

Concerning DOC, only check the .xls file it contain the list of user functions ... Other are internal ones that will not be used by AmiDARK Engine users...

Don't forget that the project is *unfinished* and it was clearly mentioned in the bounty...
If the review need to rework entirely the engine to what you think is best ... It will never be finished ..

I appreciate your ideas to improve it but, maybe the objective to see if it fit the bounty requirements must be considered first ...
And all your checking ideas do ... after..

Regards,

@AmiDARK

The dbGetPixels* functions potentially dangerous anyway, without locking the window bitmap.

_________________
This is just like television, only you can see much further.

@BSzili
if there is "db" at beginning, it's that the function is not yet available.
dbGetPixels
To what I remind, I did not yet do this command.
Where did you see it ?

@Daytona675x :
DERgbR() lowercase and uppercase are available for compatibility.
In DarkBASIC Professional / DarkGDK, both are available...

@Daytona675x

I understand your anger towards AmiDark because of his initial response to your first post in this thread, but your tone and the way you are wording all of your subsequent posts in this thread are FAR from diplomatic, and such tone and attitude make it even harder for AmiDark to consider your suggestions for code changes. It appears now that your only purpose in posting in this thread is to embarrass or criticize AmiDark's code, instead of being constructive with your review.

If you are so sure that your way of coding is so superior, now that the code is Open Source, you have the opportunity to change it your self and show your superior skills and results, correct?

I am a little disappointed that AmiDark has reacted so negatively to any code review, instead of using all suggestions to improve the code and ignoring any personal insults, either intended, or accidental in nature. I still support the completion of this project and hope that any future personal insults can be stopped, as they don't add anything useful to our tiny community.

All of this is my personal opinion, not that of a moderator (which I am no longer participating as, on this site).

_________________
Amiga! The computer that inspired so many, to accomplish so much, but has ended up in the hands of . . . . . . . . . .

@AmiDARK

Quote:

DERgbR() lowercase and uppercase are available for compatibility. In DarkBASIC Professional / DarkGDK, both are available...

In that case using some #defines to handle this would be a much better solution than having two versions of the same function.

@amigadave

Quote:

If you are so sure that your way of coding is so superior, now that the code is Open Source, you have the opportunity to change it your self and show your superior skills and results, correct?

Code reviews are not really about whose way of coding is superior, but about finding obvious flaws and bugs, and that's what Daytona has done. If you're asking for money for open sourcing a project, then at least make sure the parts of the code you claim are finished actually work like they're supposed to.

@amigadave

IMHO, Daytona is being as diplomatic as a code reviewer can or should be. Now watch this:

AmigaOS/libDos_Functions.c
MyGetFileSize has two critical bugs and one type issue:

- AllocDosObject is called with invalid input (FIBB_READ, wtf did you take that from?). It should be DOS_FIB. This leads to Examine() trashing memory, since not enough memory is reserved for the FileInfoBlock structure. You likely never even tested this - not to mention testing it with proper tools like Wipeout/Mungwall.
- The result of AllocDosObject is never checked, leading to a potential situation where ADO is called with a NULL FIB, which is undefined behavior (the autodoc does not allow this value to be 0).
- In the MorphOS version, Examine64 is used, but the result is casted back to an int - either use int64_t for file sizes everywhere, or don't use 64 DOS calls, it makes no sense

AmigaOS/Strings_Functions.c
You're using MEMF_CLEAR when allocating memory for strings. Why? This is inefficient. Since strings are 0-terminated, you only need to zero the first byte of the allocated memory.

ConvertRes2String /ConvertDateToString - is there *really* a reason to use sprintf 3 times here?

LCreateString - you use MEMF_CLEAR to allocate memory and then iterate over the array *again* to clear it by hand. Wtf? Don't you trust the OS to clear it for you when MEMF_CLEAR is being used? :) Not to mention you do not check the returned value, yet you poke the memory - this code will crash and burn should memory allocation fail. MorphOS and OS4 would most likely stop your application, original AmigaOS would most likely just die here :)

intUpCase - erm, what? Every heard of utility.library's ToUpper() ?

OK... I am running out of popcorn now...

@zzd10h

>To be reviewed by really professional developpers is a suicide.

Not at all, in fact Daytona has helped this developer and the community in general by his technical analysis. If he (or anyone else) wants to review any of my code I would be grateful rather than defensive, probably there are some bugs there but nothing like this.

It seems clear that the bounty requirements are not even close to being met and this game engine is riddled with bugs which the coder has no intention of fixing.

Last edited by Minuous on 13-Jan-2015 at 07:30 AM.

@amigadave

Not constructive? Are you serious? This is not about a "way of coding", but bugs which can cause memory trashing and crashes. I doesn't matter why he decided to do his in-depth code review, when feedback like this is invaluable.
When I got things to compile I'm going to fix the ones which aren't related to the DarkGDK API compatibility.

_________________
This is just like television, only you can see much further.

@BSzili
The main problem is that, if to get the bounty I must finish the entire engine making it as robust as possible ... I prefer to tell you directly "I give up".. Cancel the bounty and close the source code back. Because it will probably takes much time! And the main reason is that, if I created the bounty, it's not by pleasure but by needs. A true need (I'm EHS, lors my job, my wife too, a baby at home, etc.)
I currently cannot spend all my days to something that do not help me financially! (Cos I cannot work because I am in a full time formation (at home due to my EHS) for a full requalification due to my ElectroHyperSensibility that makes me lost my previous job... and it finish on August 2015. After this I can start work again with my new skills and my situation should become better.

I would be happy to fix everything that will be said, once the bounty will be completed. As I win money on it, it's easier to use that to tell to my wife "Ok, they (Amiga community) helped us a lot by acquiring the source code of the AmiDARK Engine, I can invest (this money) in time to develop more the product (on my remaining free time)"

For now, I think we should more concentrate on the bounty requirements :
There : http://www.power2people.org/projects/amidark/

- Are the 4lines concerning the "About the AmiDARK Engine" ok in conjunction with the informations given with the "current development state" ?
- Are the 6 Technical informations correct ?
- Are the project requirements correct ?

Last edited by AmiDARK on 13-Jan-2015 at 10:08 AM.

@AmiDARK

You don't have to explain the bounty conditions to me. As far as I'm concerned, you did your part, which is open sourcing your engine. I said the same over A-E when the thread was first opened. My response was to amigadave, who was eager to dismiss Daytona675x's code review as non-constructive bullying. I don't think the majority of the folks here are against you getting the bounty money.

_________________
This is just like television, only you can see much further.

@BSzili
Sorry BSZili, it was a "global" message ... I did forgot to remove the @BSZili added by the "reply" function of the forum.

Last edited by AmiDARK on 13-Jan-2015 at 11:06 AM.
Last edited by AmiDARK on 13-Jan-2015 at 11:05 AM.

@BSzili

Agreed.

I donated to the bounty and was fully aware that AmiDark bounty was based on the engine as it is.
As far as im concerned Im happy to see Amidark recive the money RIGHT now.
He said clearly before I donated that the engine was/is work in progress, which Is what we see now, with the help of these reviews in addition to Amidarks own work.

Additionally Im very happy to see the effort other developers has put in going over the code, and Ive said as much in private msg to Daytona.
I was under the impression that was part of the intention of the bounty; opensource it and let it be developed under the supervision of AmiDark.

So from where Im sitting everything is going as potrayed by the bounty and Amidark should collect the money NOW.

Last edited by Overflow on 13-Jan-2015 at 11:03 AM.

@Overflow
Thank you.


I suggest that I create a new post/subject where we'll add all these fix/improvements to do and I'll keep it updated when I'll get something fixed.
What do you all think about this ?