@AmiDARK

A development thread that is kept ON topic is a very good idea.

Makes it easier for moderators too.

@amigadave

Any offence caused seems to have blown over before your post and Freddix is looking into the bugs pointed out already.

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?

Since Freddix is still updating AmDark it's better that he is given this kind of feedback and does the fixes himself. Daytona is probably busy with his own projects so it's great that he took the time to do a code review.

@Daytona675x

i did not donate to the bounty so I personal cannot decide if it is "fullfiled" or not

But for me personal (when I read the bounty) it was always obvoius that the bounty is about opensourcing "what is available" and Amidark would continue to work on it (and not a ready-to-go engine).

So it would be helpful not to trash the engine but to give constructive advices where potential problems are and what is missing. It is good that Amidark wants to continue because there is much opensourced amiga-software that certainly has fantastic code quality but no developer interested in it. So a opensourced engine "in-development" has more use than one with "great quality" but no developers. Don´t you agree? And Amidark will then hopefully fix the problems in future.

Bitmap

Today I'm going to review the Bitmap part of AmiDARK engine.
I'm only concentrating on bugs / problemes / inefficiencies in that dedicated area.
I won't comment on inefficient but uncritical things like that for example:

bmpBitMap[ iID ].IsFlipped = bmpBitMap[ iID ].IsFlipped + 1 ;
if ( bmpBitMap[ iID ].IsFlipped == 2 ){ bmpBitMap[ iID ].IsFlipped = 0; }

bmpBitMap[8] (very bad style)
Bitmap_Variables.c, line 30
You should avoid hard global array-sizes with hard-coded numbers like the plague! Really! This is p.i.t.a. as soon as you want to change that size or if you try to find yet another index-typo inside your code. Always use a define / const for things like that like DE_MAX_BITMAPS and use that instead.
And if index 0 is somewhat special then also use special defines / consts / enums for it to clarify that (like for example DE_INTERNAL_PRIVATE_BITMAP=0 or DE_FIRST_USER_BITMAP=1, whatever). Things like that will help you a lot to avoid certain types of bugs. And it will help others who dig inside the source.

BITMAP_Constructor (very critical! initialization missing)
The global bitmap array bmpBitMap[8] is never initialized. It's initial content is pretty much random. This is highly critical because many bitmap functions rely on the correct value of MyBitMap.Exist at least. The bitmap [0] is the only one that eventually gets sort of initialized (although too late), namely in SETUP_Display.c / DESetDisplayMode.

DECreateBitmap (error handling)
I already saw other functions around the lib that behave similar like this function:
if you feed it with an ID that's already in use, then it will kind of fail / do nothing internally but it won't tell you.

// right after prog init
DECreateBitmap(1,100,100);
// now bitmap 1 will be of size 100 x 100
// ... do other complex things
DECreateBitmap(1,150,100);
// now bitmap 1 will still be of size 100 x 100

The problem is that the library does not help you to avoid such issues. Of course you can argue that you have to take care of your bitmap-IDs yourself. Okay. But that's probably not what I'd expect from a lib with the primary purpose of being noob-proof
I have no idea how the original DarkBasic behaves in that case. IMHO the best noob-safe approach would be to let DECreateBitmap resize / delete-recreate the bitmap in such a scenario. And if that's not an option due to compatibility or whatever you should at least return an error-code (or set a GetLastError int or so).

DELoadBitmap (error handling)
Similar issue to the one above. If you try to load a bitmap-file into a bitmap that already has been loaded a bitmap-file for, then it will silently do nothing instead of a) notifying you or b) replace the previous picture by the newly requested one.

DELoadBitmap (crash)
BMP_Functions.c, line 283:
NewPicture can be NULL if LoadPicture fails. But this condition is not checked, instead the possible null-ptr is dereferenced. This will lead to a crash if for example a file named Filename doesn't exist.

DECopyBitmap (buggy)
BMP_Functions.c, line 313:
Typo, apparently the Heights should be checked for equality. Instead the Widths are checked twice.

DECopyBitmapEx (dangerous, can crash easily)
BMP_Functions.c, line 348:
This function lacks any parameter validation. As a result the internal pointers may point to nirvana if the coordinates you supplied are not 100% valid. This in turn may lead to undefined behaviour / crash.
Furthermore the value TgtSizeToCopy from line 347 is not used - which shows yet another problem regarding the coordinates: if X2-X1 != ToX2-ToX1 intersting things may happen. Did the original DarkBasic version support stretching / mirroring here? If not then I wonder why they did not use width / height parameters instead. If they support stretching then this feature is missing from AmiDARK right now.

DEGetBitmapPtrEx (buggy)
As BSzili already pointed out accessing the bitmap that way isn't safe. LockBitMapTags should be the way to go.

_________________
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
As usual, you lack of knoledge concerning how DarkBASIC Professional / DarkGDK work makes you think I'm a "noob"..
And as the AmiDARK Engine is NOT finished, many features like "Error Handler" are not yet done.

bmBitMap[ 8 ] is 8 because it is a FIXED value that will NEVER BE CHANGED.
Compatibility with DBPro/DarkGDK amount of bitmap.
Not BAD STYLE but FIXED DEFINITIVELY.

Constructor will be fixed soonly;

DECreateBitmap will integrate "later" the ERROR HANDLER.
Inside DarkBASIC Pro if you try to create a bitmap that already exist, you'll get an error message. It what will be added in the future. That's why the if ( DEBitmapExist( iID ) == FALSE ){ is present. It will be used later with a }else{ to handle this kind of error.
As you noticed, everywhere in the engine, the ERROR HANDLER is NOT YET DONE.

DELoadBitmap : Same sentence.

DECopyBitmap : Will be fixed.

DECopyBitmapEx : Yes I know, in fact before the virtual bitmaps were only memory block. To be more compatible and useable from system functions, I started to migrate them to true Amiga Bitmap. I must add locking and checking to these ...

DEGetBitmapPtrEx : Yes, in fact it will be given in the HELP, like in DarkBASIC Professional, that the virtuals bitmaps must be locked before being used otherwise it can cause problems. And Unlocked when finished.

I will fix these as soon as possible but I'd like to remind that all those are "out of the bounty rules"...

@AmiDARK

Quote:

As usual, you lack of knoledge concerning how DarkBASIC Professional / DarkGDK work makes you think I'm a "noob"..

He's not saying you're a noob, but that the user of AmiDARK might be a noob, which is why simple error handling is important even at this stage.

@jacadcaps

I did not mean to say that the code review Daytona was doing was not useful, or helpful, but only that the tone used after his first message was condescending and hostile with the intent to ridicule or humiliate AmiDark's coding choices or experience. I think that the same review could be done without any comments added which imply anything about AmiDark's competency and they would be received better. Daytona's last message is less harsh than the previous one, so I probably don't need to worry about how AmiDark is going to view these reviews, and perhaps he will use them to improve the code.

I think that all of this code review is invaluable to any programmer and that AmiDark should be thankful, but it has almost nothing to do with what he asked for, which was to confirm that he has fulfilled his bounty requirements, so he can receive the money which he badly needs to survive in his current situation. As many in this thread have said already, we donated because AmiDark was Open Sourcing his code, which is a WIP, not a completed project.

I obviously want all of these problems fixed eventually and I think that AmiDark can learn a lot from the programmers who are taking their time to review this code, but I also think that he has earned the bounty money by Open Sourcing the code in it's current flawed state. Hopefully, AmiDark will not be so disappointed by the number of mistakes noted by his peers who are reviewing his code that he will not want to continue working on it to correct these and other bugs, or code mistakes over the next several months (or years), to complete the project and give the Amiga community another tool that will be useful.

Asking for this code review to see if the bounty has been fulfilled may be a great blessing in disguise for AmiDark, or any other programmer who wishes to work on this project in the future, because of the in depth review that Daytona is doing, even if the precision of that review was increased by anger toward a forum response initially.

All I commented on was the tone of the messages, not the usefulness of what was being written. If I had to choose between no detailed review of the code and a nicer tone of the messages from Daytona, and the detailed review he has given including several comments that may be insulting or harsh in the way they are worded, I would have to admit that the value of the review and pointing out important flaws in the code is more important.

Last edited by amigadave on 13-Jan-2015 at 07:14 PM.

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

@all

It looks like everyone agrees that the code that he released is in fact the code promised in the bounty. So, he's fulfilled the bounty's requirements, and it's time for him to get paid.

Having people reviewing the code in detail is great, but that's beyond the scope of the bounty.

Hans

_________________
http://hdrlab.org.nz/ - Amiga OS 4 projects, programming articles and more. Home of the RadeonHD driver for Amiga OS 4.x project.
https://keasigmadelta.com/ - More of my work.

@amigadave
Quote:

but only that the tone used after his first message was condescending and hostile with the intent to ridicule or humiliate AmiDark's coding choices or experience.

I don't know where you get that idea from, but if you carefully check out the discussion's flow you'll probably find out that it was AmiDark who reacted pretty "hostile" when pointed to just a few of his code's bugs.
I have to admit that my voice "raised a bit" in form of a tad more sarcasm as a result to his ridiculous reactions, but that was in the immediate counter-posts back then. I really don't see where I can be considered hostile or whatever in any way in my posts since then. Actually I'd say I'm pretty friendly to him, especially considering his ongoing blurp against me. Luckily I'm not that thin-skinned as him
Anyway, as I already said the intention of this review is to give people an idea of the code's quality and to help improving it, a fair review. Sorry if AmiDark doesn't like it too much.

Quote:

but it has almost nothing to do with what he asked for, which was to confirm that he has fulfilled his bounty requirements ..... I also think that he has earned the bounty money by Open Sourcing the code in it's current flawed state

Well, that's your opinion. Me - and apparently not just me - think, as mentioned a few times now, that at least some minimal code quality can be expected if somebody sells code this way. Sadly there is no such minimal quality at the moment. That's what I'm pointing out. I find it unfair towards the paying people to present them such a, sorry, mess.
Btw.: it's not AmiDark who asks for a review... It's Power2People and the people who paid for this thing and some forum guys.

Quote:

so he can receive the money which he badly needs to survive in his current situation

Accepting the bugs existance and fixing them would be a good idea then. Or at least saying "sorry people, I thought it was less messy but was wrong. I'll fix it in the future".
If his current financial situation is bad, sorry to hear that. But that shouldn't automatically mean that the donating people should have to accept this instable code only to help him out.
My opinion is: if he had clearly stated in big fat letters on top of the bounty "this is a still very unstable und buggy prototype, please help getting it on track" this would all be a totally different story.


@AmiDARK
Quote:

As usual, you lack of knoledge concerning how DarkBASIC Professional / DarkGDK work makes you think I'm a "noob"..

I have no idea what you're talking about.

Quote:

And as the AmiDARK Engine is NOT finished, many features like "Error Handler" are not yet done.

That's not good.

Quote:

bmBitMap[ 8 ] ... Not BAD STYLE but FIXED DEFINITIVELY.

If it is fixed forever (until the DarkBasic guys change it ) doesn't really matter when it comes to the more important part of my statement you seem to have overread: a define/const would help you to make less mistakes. Some of the index-bugs you definitely made would most likely not have happened if you hadn't used hard coded 8s or 9s everywhere. But you're right: it's not bad style. No, it's the worst possible style

Quote:

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

No, as I have shown it is broken. But yes, it also needs optimisations. What's "fixed project" supposed to mean?

Quote:

Other are internal ones that will not be used by AmiDARK Engine users

? Internal functions will be used by AmiDARK Engine users. Only indirectly.

Quote:

- 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 ?

You have to be really, really, really nice to accept this.
For example this one here:
Quote:

The engine contains a partial HTML documentation explaining how the commands / functions work (632 commands / functions done)

See, you can describe something the way it is - or sugarcoated.
The real description would be: 632 commands have very rudimentary undetailed one or two sentence descriptions.
You can also sugarcoat things by simply not writing about its limitations. That's what this "technical information" does.

Quote:

Don't forget that the project is *unfinished* and it was clearly mentioned in the bounty...

Not clear enough IMHO. There is a difference between "unfinished" and "unusably unstable".
There is a word for this kind of stuff and it is mis-selling.

@Hans
Quote:

It looks like everyone agrees that the code that he released is in fact the code promised in the bounty.

Haha, don't know where you got that idea that "everybody" agrees on that. Just for the record: I don't agree. This is not what was promised.
Luckily it's not about my money

Last edited by Daytona675x on 13-Jan-2015 at 08:21 PM.
Last edited by Daytona675x on 13-Jan-2015 at 08:20 PM.

_________________
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

Took a quick look, and the sprite drawing looks extremely inefficient.

If I understood correctly (quick look...), it reads underlying area to ram, and draws it back when "erasing" sprite, instead of something more efficient (such as just redrawing the screen) - don't know if such more efficient methods are impossible for some reasons that I didn't notice

@Jupp3
OpenGL 2 (that is our Amiga standard) does not allow something better for sprites.
I may have "never added them" but I decided to add them only for compatibility with DBPro/DarkGDK.
More of this, I've looked everywhere on the net to find how we can do perfect pixel system under OpenGL and even all the informations given on the net. It's impossible to reach on Amiga OpenGL (or I didn't found it yet).

@Daytona : Maybe you should look into "your" words and imagine you are me and not you ... and even if you do this, I think you will not see your "negative" and "pejorative" words like saing "my ridiculous reaction" .. It's a lack of respect. But it's Human to never see how we interact with others .. we always think "I am in my right to tell this" ... and it's a pain ... it's what makes our world so empty of love and compassion ... And why there are war (of intentions)...
However, like I said, your ideas for improvements are really good but I don't agree with you concerning the engine stability.

Last edited by AmiDARK on 13-Jan-2015 at 09:20 PM.
Last edited by AmiDARK on 13-Jan-2015 at 09:19 PM.

@All
Sources are available = check
Examples / demos can be compiled ?

If yes I see no problem.

_________________
AROS Broadway - AEROS - Aminux - AmiCloud - indieGO! Appstore - AmiWallet - VAN lossless video codec - AMC Amiga media Center -KrypUnite - LibertyNet - MinX - amigaNX

Let the man get his bounty and start working on the project ! What put him down fest is this ?

Last edited by Tomppeli on 13-Jan-2015 at 10:50 PM.

_________________
Rock lobster bit me. My Workbench has always preferences. X1000 + AmigaOS4.1 FE
"Anyone can build a fast CPU. The trick is to build a fast system." -Seymour Cray

@phoenixkonsole

+1

There was no mentioning of "code quality" in the bounty. I think it was clear that you get what is there. And 1500 EUR is not much, that is payment for one week

@Daytona

you write a engine (whatever code quality) in one week for that money? Really? Wow

you have to see the amount of money and then accept. I did not make a donation so it is a little funny if people who did not donate define if it should be paid. Similar happened at the Aros port of Magellan and the Magellan bounty, people not donating were suddenly heavily involved.

We all understand now that you are not very excited about the project but the donators should say if money should be paid and not I or you.

@OlafS25

Good idea.

@all donators

Do you agree that AmiDark completed the bounty ?

Please answer in this thread to finish this potentially demotivating thread (for AmiDark)

I donated and I consider the bounty completed.

Results ( Completed / Not completed) :

1 / 0

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

@zzd10h

As said earlier, im a donator and consider the bounty complete. Why is it even a discussion?
It reached over 100% in the allocated time. Case closed.

That said, AmiDark should make a new thread that is spesifically about development and code related feedback regarding the source/engine. I realise this thread was intended as such.

I do have to say; part of the problem in THIS thread is that AmiDark keeps misinterpering Daytonas intentions and spends energy defending himself when he really shouldnt do that.
Maybe its because I got a rather thick skin, but I thought Daytonas first post was quite to the point.
Wether or not he was correct in all his points regarding his review(s) is besides the point.
Read his comments, extract the useful feedback and respond on topic what the intended function is. This will help efficient two way communication.
Tone and intent is often hard to read on forum.

THIS thread as gone way off course, so can you please make a new thread where the people with programming skill discuss the enigine, while we coding illiterate refrain from posting unless we got ON topic questions.

Last edited by Overflow on 14-Jan-2015 at 04:36 AM.
Last edited by Overflow on 14-Jan-2015 at 04:12 AM.
Last edited by Overflow on 14-Jan-2015 at 04:11 AM.

>However, like I said, your ideas for improvements are really good but I don't agree with you concerning the engine stability.

>Let the man get his bounty and start working on the project !

It seems obvious that if he won't admit there are bugs and fix them when a large amount of money is at stake, he is hardly likely to do so afterwards. My prediction: bounty will be paid and then nothing will happen afterwards.

@Minuous
Who cares! Amidark was for sale and the community decided and "suggested" him to stat a bounty.

Hans started the drums to reach the target sum.
He points to videos and demos.

Most donors are non coder and it seems those demos looked exciting enough.

So if those demo examples can be compiled I think the bounty is completed.

It must be clear that there is a risk that the original author may decide to drop it because in worse case he would have sold it to a comercial entity like me.

And what I can see here is that other developer can check and optimize the code anyway....
If desired or paid for.

Who knows maybe someone can do some 3d demos based on the available ones or in best case Amidark continuous his work...

All tht doesn't matter..

Bounty = community suggestion
Reaching the sum = based on Hans advertising with available videos /demos
Source code released = yep
Can the shown demo examples be compiled or is there anything broken?

Everything else doesn't matter. Just prove the compilation with a video or maybe some reviewing coders can try that.


--------
Let's say I release solid works source code .... Which is not as fast as catia v5..... Makes this the slower code worthless?

Last edited by phoenixkonsole on 14-Jan-2015 at 06:55 AM.
Last edited by phoenixkonsole on 14-Jan-2015 at 06:49 AM.

_________________
AROS Broadway - AEROS - Aminux - AmiCloud - indieGO! Appstore - AmiWallet - VAN lossless video codec - AMC Amiga media Center -KrypUnite - LibertyNet - MinX - amigaNX

@Overflow

2/0

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

Compilation OK for few samples under AmigaOS4.
Unfortunately, without Warp3D, I'm unable to start them. I was aware of this fact.




If somebody could test this executable under AOS4

http://zzd10h.amiga-ng.org/AmiDark/BoingBall.lha


@AmiDark
You should remove the hard coded CodeBench path in all your cbp .info and rename the makefile.os4 to makefile.
It will be easier to compile under OS4

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