icon.library 46.4 test versions

[PeterK]

Originally Posted by pipper

I would try that, but I don’t have a record of what your patches are.

You can find all my changes in the two subroutines backdrop_show_drag() and backdrop_stop_drag() if you compare DOpus5.82originalCode.asm with DOpus5.83fixedMemLeak.asm. Most differences are just optimizations made to get some free space to insert my patch code without changing the size of these routines. Then I compiled the fix with PhxAss and copied and pasted the binary code with FileX into Dopus at the right offset. So you need to deal a bit with asm code to understand my patch.

WRT building DOpus: don’t be afraid. This is super easy, download Bebbo’s toolchain, let it build itself, download DOpus, let it built and off you go.

Yes, I could try that, but I have only very little experience in C coding and really don't like messing around with all this inefficient compiler code juggling. The biggest disadvantage of recompiling Dopus is that I can brake everything in the program and all the other functions and modules if I make a mistake in my compilation (you know, murphy is my best friend). With my little asm patch I only risk to get new issues in the dragging routines, everything else remains untouched. And I don't want to test all the features of DOpus5 for possible new bugs.

[midwan]

I'd be happy to build DOpus from the C sources also, as I have more experience in C/C++ than in Assembly (almost none!).

[midwan]

Regarding the toolkit to build: If you're using Docker, things are actually even easier. There's a readily available image that will get you the compiler environment: https://github.com/AmigaPorts/docker-amiga-gcc

[PeterK]

@midwan and pipper

Thanks a lot for your links to dev kits and tool chains, I will have a look at them, but I don't want to promise at the moment to compile Dopus 5.9x. Have just installed 5.91 and are playing a bit with it to find out what has changed since 5.82. The custom dragging seems to use no image masking at all anymore or there is something different in my settings (as I hope). I still remember from AmiKit 8 that some things are even worse in 5.9x than in 5.82.

[midwan]

I compiled Opus5 from the source, based on @pipper's repo. I used the docker image I mentioned above, with a couple of modifications:
- xadmaster headers were required/referenced, but not included. I dumped them in the source/Include directory to get over that quickly
- zip was not included in the docker image, but I've added it (and requested that it's added in the image as well)

Otherwise, it went well and I got a full release out normally.

I've hopefully implemented the fixes correctly, at least in a few quick tests I did on my A4000 (same setup I was testing before also) it seems to work without problems.
I only changed the two lines @PeterK mentioned:

1) in

backdrop_show_drag()

replaced

AddBob(&object->drag_info->bob,&GUI->drag_screen_rp)

with

AddVSprite(&object->drag_info->sprite,&GUI->drag_screen_rp)

2) in

backdrop_stop_drag()

replaced

RemBob(&object->drag_info->bob)

with

RemVSprite(&object->drag_info->sprite)

I'm attaching the result here, so more people can test also. Please let me know if it breaks anything I missed!

@pipper: I can send you this as a PR if you want, however if this works, it should really be pushed upstream somehow. Is the sourceforge repo even maintained anymore? I haven't seen any activity there in a loooong time

[PeterK]

Oh no! Your description sounds as if you've replaced AddBob() and RemBob(). I hope you did not, because that is still required for the normal dragging routines, but I have not read the source of 5.9x yet. And I also did not test your result.

AddVSprite() and RemVSprite() should only be used for the custom dragging, not for the normal mode.

AddVSprite() is called at the same location where AddBob() can be found, and I'm converting Width into Words/Row before the call and restore Width after it, because Dopus is working with Width. After AddVSprite() I also set a new drag flag DRAGF_CUSTOM_VSPRITE, bit 31, which has to be defined in C (drag.h and dopus_base.h), of course. Feel free to create your own flag.

RemVSprite is NOT done where RemBob() exists, but just before FreeDragInfo() removes all the drag_info structures and only if my flag was set, which is cleared at that moment.
http://eab.abime.net/showpost.php?p=...postcount=3388

[midwan]

Ah, I misunderstood then.
It does work when custom drag is enabled, with no memory leak as far as I can see, but I didn't test the "normal" drag routines.

I also tested 5.91 and 5.92, both have the same memory leak problem otherwise.

[PeterK]

No problem, just tested your version and can confirm that the memory leak seems to be fixed and in spite of my assumption there are no sticky images left behind on the window frames or in other windows after the dragging, so the 5.92 code might work a bit different than 5.82. But the normal dragging mode shows no icons anymore while you move them around, because in that case they need to be real bobs.

Unfortunately, it seems that somebody has removed the image masking for the dragged icons in the custom mode of 5.9x, because I always get a complete gray rectangle now as icon background, which is much worse than it was in 5.82.

Where did you download your 5.92 source code? I've seen only 5.91 for OS 3.x.

[midwan]

I just forked pipper's repo...
Although the last binary release was 5.91, the sources were further updated and carry the 5.92 version.

[AMIGASYSTEM]

Peter excuse my ignorance, what does it mean and what is the function of this command:

If $AfA EQ ON

Without this command Eastern does not activate from my user-startup

[PeterK]

I have 3 locations in my startup-sequence which need to be changed if I want to run AfA_OS on my OS 3.1.4/3.9 system for some tests. As you already know, I'm a lazy guy, so at the beginning of my startup-sequence there are some script variables that I can set on demand:

;Set AfA ON
;Set DEBUG ON
;Set Interactive ON

These variables are working as switches for if-then-else-endif clauses (except Interactive). After enabling the line "Set AfA ON" all the parts of the startup-sequence which need to be changed for AfA_OS get activated:

If $AfA EQ ON
   MoreLibSpace >NIL: REBOOT
EndIf

...

If $AfA EQ ON
   AfA_OS_Loader >NIL:
Else
   Run >NIL: VisualPrefs
EndIf

...

If $AfA EQ ON
   Wait 2 ; wait for Eastern
   RemLib >NIL: icon.library
   KeepPlanarImages
EndIf

EndCli

Ok, now I understand, you have Eastern in your user-startup, the same place where you have RemLib. I have Eastern in WBStartup (which is started after LoadWB), because I don't like to have a user-startup at all. That explains it. As we say: there are many ways or roads leading to Rome.

[AMIGASYSTEM]

Originally Posted by PeterK

Ok, now I understand, you have Eastern in your user-startup, the same place where you have RemLib.

Not, in my AfA One Eastern is placed in WBStartup folder "Enable"

That explains it. As we say: there are many ways or roads leading to Rome
.

I didn't know that's what they say in Germany too.
Just think, in ancient times my city was the only one to be connected to Rome with 2 Consulars, the first and the most important, Via Appia and Via Traiana.

Via Appia, Via Traiana

[pipper]

WRT DOpus5 custom drag'n'drop memory leak, I debugged the issue and found a proper(?) fix for it:
https://github.com/mheyer32/dopus5al...3cf2a923d04f72

[PeterK]

Thanks pipper, your diff file and the description are looking promising, although I didn't test anything yet due to the lack of a ready to use compiled DOpus5 executable (and I still have no tool-chain installed)

Ok, can't see that you converted the width into words/row and later back, but that may not be required by AddVSprite(), probably only AddBob() needed words/row, because otherwise the AllocBitMap() of P96 gets 16 times too large. The strange thing is that these parameters like width are stored in the VSprite structure and not as part of the Bob data, but I'm no expert for bobs and sprites at all.

The other thing I made different is my new flag DRAGF_CUSTOM_VSPRITE, which should ensure that RemVSprite() only removes what my AddVSprite() has created before. But again, I'm not sure whether that is really required or was just a bit paranoid.

Did you also try to revert the changes from 5.82 to 5.9x for the image masking of the dragged icons? Now there are full gray rectangles as background, which is much worse than before.

[pipper]

Originally Posted by PeterK

Thanks pipper, your diff file and the description are looking promising, although I didn't test anything yet due to the lack of a ready to use compiled DOpus5 executable (and I still have no tool-chain installed)

Ok, can't see that you converted the width into words/row and later back, but that may not be required by AddVSprite(), probably only AddBob() needed words/row, because otherwise the AllocBitMap() of P96 gets 16 times too large. The strange thing is that these parameters like width are stored in the VSprite structure and not as part of the Bob data, but I'm no expert for bobs and sprites at all.

The other thing I made different is my new flag DRAGF_CUSTOM_VSPRITE, which should ensure that RemVSprite() only removes what my AddVSprite() has created before. But again, I'm not sure whether that is really required or was just a bit paranoid.

Did you also try to revert the changes from 5.82 to 5.9x for the image masking of the dragged icons? Now there are full gray rectangles as background, which is much worse than before.

I didn’t figure out which allocation was the leaking one. All the ones that DOpus manually did, it freed. So my assumption is that AddBob does something in the background. And maybe that was also the allocation that blew up by 16x.

This whole messy situation is only because DOpus wants to reuse the GELs linked list. Maybe we can further trim it down and just do manual list insertion and removal like shown in RemoveDragImage(). For some reason backdrop icon dragging is handled differently than others and does not use RemoveDragImage(). No clue why...

Do you know what the business of the “custom dragging” is in first place? It seems to be only applicable with RTG...

WRT masking: I don’t know that specific issue. Maybe describe it here or file a bug on github?
I don’t usually have lots of time, so it may take some time to get around fixing it.

[PeterK]

Originally Posted by pipper

I didn’t figure out which allocation was the leaking one. All the ones that DOpus manually did, it freed. So my assumption is that AddBob does something in the background. And maybe that was also the allocation that blew up by 16x.

It was the AddBob() call with "Width" instead of "Words/Row", which let(s) P96, that has patched the graphics.library functions, emit AllocBitMap() with a 16 times too large "SizeX" and consequently a huge AllocVec() like 150-256 kB for each image. You can watch or monitor that in all details with LibSnooper.

Do you know what the business of the “custom dragging” is in first place? It seems to be only applicable with RTG...

WRT masking: I don’t know that specific issue. Maybe describe it here or file a bug on github?
I don’t usually have lots of time, so it may take some time to get around fixing it.

The standard dragging routines of Workbench can only handle planar images and thus you have to convert TrueColor images or palette mapped images into planar images first, which is both, very slow and very ugly.

Of course, Dopus was never made for TrueColor icons, but it already has some support for TrueColor screens built-in, unfortunately without using the Alpha channel data for a smooth image fading against the background. In 5.82 it only uses RGB data and the background color (gray) for the image masking. Just drag an icon partially over another one, but don't drop it there, and you should see the masking. Compare that with 5.82, 5.83 and 5.9x and you will see the differences. 5.9x seems to have no masking anymore and the result is just a gray rectangle as background for the dragged icon. I will try to find the masking routine of 5.82 soon again and compare it with the newer but worse code of 5.9x.

The mask is build by a call to the library function GetDragMask() in Program/backdrop_render.c in 5.82, but I don't know whether you have enabled "USE_DRAWICONSTATE" instead in 5.9x, which did not exist in the 5.82 code.

Where is your new executable of DOpus5 ?

[Weaselrama]

I've been following this discussion for a while now and having installed AfA OS 4.8 recently, I'm curious as to what "Eastern" is. I don't see it in the AfA OS 4.8 installation. What does it refer to? Thanks.

[PeterK]

http://aminet.net/package/util/wb/eastern

Eastern (a wrong translation of the German "Ostern") was made to display thumbnail preview images of your pictures as deficons. On OS 3.x Eastern usually creates palette mapped icons and uses dithering for the color reduction of TrueColor images. The resulting quality is not looking very good. On OS4 with an icon.library version of 51 or higher Eastern can transfer the downscaled thumbnail images directly in TrueColor to icon.library and the result looks perfectly.

AfA_OS and my icon.library (v51) both offer support for Eastern to generate TrueColor thumbnail images even on OS 3.x for getting the best quality. The problem with DOpus5 is that it has replaced the support for the deficons hook by its own internal filetype system. Bernd Roesch (AfA) has written a small patch for DOpus 5.82 (which still works on my 5.83) in order to give DOpus5 the support for DefIcons and Eastern back, but with the drawback that the filetype system won't work anymore.

[James]

Originally Posted by PeterK

AfA_OS and my icon.library (v51) both offer support for Eastern to generate TrueColor thumbnail images even on OS 3.x for getting the best quality.

When I run Eastern on OS3.5 with your icon.library all I get is grey boxes with the image type and size written on them.

[PeterK]

Use the tooltype "PROGRESSIVE=NO" in Eastern.info and start it after DefIcons in WBStartup (see documentation). I would recommend the picture datatype 45.17 for best scaling quality, 46.13 works too, but with lower quality, other picture.datatypes may not work at all.