Recent

Author Topic: Graphic changes  (Read 19637 times)

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Graphic changes
« on: October 21, 2014, 12:13:12 am »
   Lazarus version 1.2.6 users that are tired of graphic related bugs and slowdowns have the opportunity to replace some of them with other bugs and other slowdowns.  :o
   All you have to do is extract the archive in the Lazarus directory and then rebuild Lazarus. It's easy. :)
 ;D

A patch for svn revision is added here:
http://forum.lazarus.freepascal.org/index.php/topic,26199.msg161684.html#msg161684
« Last Edit: January 04, 2015, 12:17:55 pm by lagprogramming »

Mike.Cornflake

  • Hero Member
  • *****
  • Posts: 1260
Re: Graphic changes
« Reply #1 on: October 21, 2014, 03:08:09 am »
Do you have a list of changes?  I note you have some entries in Mantis.   Is this an agregate of those, or is there new fixes in here?
Lazarus Trunk/FPC Trunk on Windows [7, 10]
  Have you tried searching this forum or the wiki?:   http://wiki.lazarus.freepascal.org/Alternative_Main_Page
  BOOKS! (Free and otherwise): http://wiki.lazarus.freepascal.org/Pascal_and_Lazarus_Books_and_Magazines

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Graphic changes
« Reply #2 on: October 21, 2014, 09:06:46 am »
   Do you want me to attach a file with comments?
   After changing the file I think you may have to check many mantis bugs related to graphics.  :D

Mike.Cornflake

  • Hero Member
  • *****
  • Posts: 1260
Re: Graphic changes
« Reply #3 on: October 21, 2014, 10:01:17 am »
   Do you want me to attach a file with comments?

Not for me thanks, I don't have time to go through your code and compare it with existing...   

I think it would be better if you uploaded a series of dedicated patches to Mantis (or even one large patch).  You'd need to be specific on changes as you're asking others to do a LOT of work if you do it that way. 

My concern is one of longevity of your work.  As you've done it now, it's valid for entirely as long as the Lazarus Team don't make changes to their own copy of this unit.  (essentially you've forked this unit :-) ).  There may come a day when users of your version are unable to copy the file over the existing without encountering compiler errors.

Entirely up to you though.  I'm not doubting you've done good work, and I appreciate the spirit of sharing.  If you're happy maintaining a forked unit, go for it :-)
Lazarus Trunk/FPC Trunk on Windows [7, 10]
  Have you tried searching this forum or the wiki?:   http://wiki.lazarus.freepascal.org/Alternative_Main_Page
  BOOKS! (Free and otherwise): http://wiki.lazarus.freepascal.org/Pascal_and_Lazarus_Books_and_Magazines

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Graphic changes
« Reply #4 on: October 22, 2014, 07:43:35 pm »
   I agree with you, but unfortunately main developers are not satisfied with the way I work.
It looks to me like I bother them with my proposals. At the same time, I don't see things the way they do.
   It's one of the reasons I try to support the community without their help.
   Anyway. I upload an archive for lazarus 1.2.6 users. Many graphic related bugs are affected.
The ones that had to work with some bugs(not only mantis reported) have the possibility to try something new, maybe a workaround lies in this archive.
   I wish them good luck!

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Graphic changes
« Reply #5 on: October 23, 2014, 06:09:00 am »
   I agree with you, but unfortunately main developers are not satisfied with the way I work.
It looks to me like I bother them with my proposals. At the same time, I don't see things the way they do.

Well so far I have seen nothing that would make me download your files in your messages. Just some generic mention about an alternative way to do things bugs and slow downs that are not mentioned specifically.

Of course I will not download or even look at your code if there is nothing in the description that captures my eye I have enough of my own code to write/correct to spend time in something else.

So you should edit the first post add as many of the bugs that are corrected as possible with a short description (one or two sentences nothing more) and the link to the bugtracker report of the bug. also what is speed up and how eg if you changed the logic behind the procedure used using a new algorithm then that algorithm should be named or just state not described though. Just something I can read in less than 2minutes and see if I'm interested to look in the code.

After that we can talk about what you should do to make your changes available to the main developers in way that they will be interested to spend some time in validating your solution, but as it is now you have no chance on people looking at your code.
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64


marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: Graphic changes
« Reply #7 on: October 31, 2014, 11:10:41 am »
Most of those bugs have no patches attached?

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Graphic changes
« Reply #8 on: November 01, 2014, 05:21:31 pm »
   And some notes that may need further attention:
gtk2winapi.inc.function TGtk2WidgetSet.PostMessage within "CombinePaintMessages" something looks wrong when "(NewMsg^.Message=LM_GTKPAINT)and(OldMsg^.Message=LM_GtkPAINT)" because we just copy data into local variables.
gtk2winapi.inc.function MessageButtonClicked Looks like it doesn't do what is advertised. Always returns false.
customform.inc. function TCustomForm.CloseQuery: boolean; might not return a result.
   And by the way, the two archives have been updated.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
Re: Graphic changes
« Reply #9 on: November 01, 2014, 08:23:44 pm »
Most of those bugs have no patches attached?

No, they are all without a patch.
Lagprogramming, can you please explain why you don't fix any of those bugs?
Instead you blame the developers for treating you bad, which is nonsense.
You have provided 5 patches. Most of them were minor cleanup and useless formatting.
Still almost all of them were applied, as a sign of good will and to encourage you to also fix bugs.
For example this one I applied against Vincent's will :
  http://bugs.freepascal.org/view.php?id=26885
Do you now feel insulted because I mentioned that formatting changes are not important and we would need bug fixes instead?

Only this one maybe fixed a small bug but I am not sure:
  http://bugs.freepascal.org/view.php?id=25693

I did not look at your code now but I don't believe it fixes any bugs.
Otherwise you would have mentioned it clearly.
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Graphic changes
« Reply #10 on: November 02, 2014, 02:27:55 am »
@JuhaManninen
   “You have provided 5 patches. Most of them were minor cleanup and useless formatting.
Still almost all of them were applied, as a sign of good will and to encourage you to also fix bugs.
For example this one I applied against Vincent's will :”
   This means that you deserve an explanation.
   In order to explain you'll need more than two minutes to read, but that's nothing compared to my efforts of writing this. :)

   "Lagprogramming, can you please explain why you don't fix any of those bugs?"
   1) We don't share the same interests. You are interested in closing as many bugs possible, I'm interested in code execution speed.
   I lost hope in a good lcl many years ago. The antidote I have for the bugs: I use only basic components and even those might be overridden. For example, a TListbox might be completely drawn by my application(100%, even the text characters don't depend on existing fonts installed). At this level I doubt that mantis bugs will affect me much.
 So, I don't have a personal interest in closing mantis bugs.
   But this means I'm interested in speed, obviously I've found lots of things that can be improved both in Fpc and Lazarus, things that I've considered can be applied without a big effort from main developers. This is the reason why there were PROPOSALS of minor modifications. If these minor changes are not welcome, what about the big ones!? Those were PROPOSALS, nothing more. I don't expect you to agree with them, you just accept them or not, I don't feel offended no matter what decision you take.
2) You have many bugs that apparently share the same source, but the way lcl is designed makes debugging very hard and you know it. This is why I consider it's useful to pinpoint the areas with problems. Even a function result that may not be assigned is important because nobody can say what is the impact on the bugs(filled or not within mantis). But simply pointing such minor problems will make main developers pretend a patch from the reporter or even worse(developers closing tickets not only without solving the problems, but stating that it's not a problem at all). So, not only that I don't have a reason to fix bugs, but now I have no reason to report within mantis my findings.
3) Many bugs hide additional problems, problems that are hard to spot. Look at the comment I've done at the “0021589: TBitmap.SaveToFile does not work with monochrome Bitmaps” report. It's enough to add 10 characters in the source file to close the ticket, but I've noticed additional problems there. Closing the ticket will hide those problems and again many years might take to implement 64bit bitmaps there. People won't know where is the problem. The transparent bitmaps, again, it remains a workaround, because the original function is so ugly I'd rather stare at the sun than at it. Why should I offer a patch to main developers with a workaround for the bug, knowing that the patch will make even harder to debug other bugs(including not yet reported ones). Also, in my work I don't connect my findings with existing bug reports. For example, I remember that there was a function that didn't created a palette for monochrome bitmaps  I think, but without connecting this situation with a ticket, I would have disturbed main developers again. Later I've found another ticket something related to a form save or load, I don't remember now exactly, it was related to monochrome bitmaps.
4) You complain about those minor proposals. If you accept a proposal, I have to keep track of it until next stable release and immediately after for a while. That takes me much more time than you need to analyze the proposal. So, each time I propose something to you, if you accept the change I'd have to work harder than you. For example, between the last stable Lazarus and trunk only about 10 functions/procedures have been modified by both me and main developers. The files presented have more than 200 modifications. Do you realize what would happen if I'd propose all of them to you and you'd accept all of them, each and every one of them with minor variations!? :))

   “Instead you blame the developers for treating you bad, which is nonsense.”
   I don't blame developers for not sharing the same interests or the same points of view. At the same time main developers should accept that I can offer support to the community without their help. However I can't say that I'm very happy with situations like:
Proposal report for zero and near zero comparisons where main developer says that it's fixed.
“0026794: Slow code generated for for near zero{-1,1} comparisons [feature]”
New report without any response.
“0026925: "For" loops optimizations proposal [feature]”
I tell you what this means. I've written code according to what the main developer said and after a while I've discovered that the code produced was not optimized. I had to modify again the code to manually optimize the zero comparisons. A lot of work for what, and some of you complain about 2 minutes?! Is this reasonable?!
What about this report.
“0026089: Slow code generated for if...then...else”
The support for the workaround came from ordinary developers like me, not from the main ones. That's not a problem, however, the discouraging attitude of the main developers is something unpleasant but probably necessary :). Main Lazarus developers do the same, so it's not like I lack experience in this field.
Last time I've checked the following function was 15 times faster than original.
Code: [Select]
function TFPReaderBMP.CountBits(Value : byte) : shortint;
var i : shortint;
begin
result:=0;
for i:=0 to 7 do
  begin
    if (value and byte(1))<>0 then inc(result);
    value:=value shr 1;
  end;
end;
   Why should I fill a ticket knowing that my proposals are not welcome?!

“Do you now feel insulted because I mentioned that formatting changes are not important and we would need bug fixes instead?”
   We have different understandings regarding formatting. By formatting change I understand lines, spaces, comments...modifications that don't influence binary produced code.
Formatting change:
1)var x:integer
   y:integer;
2)var x,y:integer;

Examples when binary code produced differs(meaning that it's not just a formatting change in my point of view):
1)Begin x:=10;if y=0 then exit;z=6;end;
2)Begin if y=0 then exit(10);z=6;end;
or
1)if pointer=nil then Result:=0 else Result:=1;
2)if pointer<>nil then Result:=1 else Result:=0;


   Personal conclusions:
   I think there are few Fpc/Lazarus main developers really active. Looking at the code and at the attitude I have reasons to believe that the number won't greatly increase in the future. I consider Fpc rtl sources structured and optimized much better than Lazarus's lcl. I realize that a new developer might have a shock looking at the lcl. I think the way the code is structured is prone to bugs, reason why you have so many reports, bugs that are harder to pinpoint compared to fpc's rtl ones.
   Example regarding code structure:
procedure DefaultReaderDescription(AWidth, AHeight: Integer; ADepth: Byte; out ADesc: TrawImageDescription);
  The way it's structured, is prone to bugs. There are many routines like this one. This procedure needs total rewrite in my point of view. The files provided by me contain a workaround for one of the reports(static text I think) where this procedure has been modified, but the solution lies in rewriting the procedure so that you won't initialize with default 24bit values and after that modify some of those values as it does now.

“I did not look at your code now but I don't believe it fixes any bugs.
Otherwise you would have mentioned it clearly.”
   Anyway, the modifications are not done for main developers and are not intended to close bugs. These correlations appeared as a side effect. Also, I don't need reviews from main Lazarus developers regarding the code modified by me as I know very well what I've modified and why. I just offer an alternative to those that need a little bit of snappiness from the lcl and some additional informations to main developers that may be interested. FOR MAIN DEVELOPERS THE PASCAL CODE IS USELESS, IT'S THE FINDINGS THAT MAY MATTER, like those in P.S.. If I do it in mantis you disagree because I occupy your time, if I do it on the forum you also disagree. It's not like I earn something from any of you, and also you are free to take whatever you find useful from my modifications. I don't care if any of you uses anything at all from these files, meaning that I also don't care much about texts like:
“After that we can talk about what you should do to make your changes available to the main developers in way that they will be interested to spend some time in validating your solution, but as it is now you have no chance on people looking at your code.”

P.S.
I wrote this text off-line so I don't know if I mentioned these before:

procedure TBitmap.LoadFromStream(AStream: TStream; ASize: Cardinal);
It might not work when “{$IFDEF ENDIAN_BIG}”. Swap might require to be treated as a function not a procedure.

function TWinControl.SendDialogChar(var Message : TLMKey): Boolean;
Original function didn't always returned result. Could it be related to a bug, something like: when I open  the program if I keep shift pressed the key is not detected, or something like that?!

function TGtk2WidgetSet.GetClipBox(DC : hDC; lpRect : PRect) : Longint;
What happens if "lpRect=nil and IsValidDC(DC)??? TGtk2WidgetSet.GetRgnBox checks for nil better than this function.

function TGtk2WidgetSet.GetClipRGN(DC: hDC; RGN: hRGN): Longint;
A bug may be introduced here. It is advertised that the function has to return values {-1,0,1} but default value is 2(SIMPLEREGION).

User137

  • Hero Member
  • *****
  • Posts: 1791
    • Nxpascal home
Re: Graphic changes
« Reply #11 on: November 02, 2014, 08:48:25 am »
About formatting, this one seems off. Let me mark indent begins and ends
Quote
function TFPReaderBMP.CountBits(Value : byte) : shortint;
var i : shortint;
begin
result:=0;
for i:=0 to 7 do
  begin
    if (value and byte(1))<>0 then inc(result);
    value:=value shr 1;
  end;
end;
That's as far as eye sees it, but clearly if i thought it this way, i wouldn't be able to distinct that the last end; belongs to procedure. Why can't we all just have this:
Code: [Select]
function TFPReaderBMP.CountBits(Value : byte) : shortint;
var i : shortint;
begin
  result:=0;
  for i:=0 to 7 do
  begin
    if (value and byte(1))<>0 then inc(result);
    value:=value shr 1;
  end;
end;
Alternatively putting "begin" right after "do" would still keep the indents right. There should be indent after begin always, even when it belongs to function or procedure word. Much easier to read code.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
Re: Graphic changes
« Reply #12 on: November 02, 2014, 01:25:07 pm »
Yes, LCL code has its issues. Lots of glue code is needed sometimes and the code is messy.
However it is not improved by micro-optimization as you are doing now. For example in the issue #26885 you eliminated temporary values stored in a variable.
The compiler can already do such micro-optimization, you don't need to worry about it.
As Vincent mentioned the original code was better for debugging, but I applied it mostly to make you happy. For some reason you are still not happy ...
In some patch you found an uninitialized variable. That was a bug, please keep reporting those findings. Thanks.

I looked at your FPC if..then..else issue. Comments from the FPC developers seemed reasonable. You again concentrate in wrong things.

If you really can speed up some relevant code *15 times, please report it with a patch and a test program. I believe it will be accepted. Somehow I am suspicious about your speedups though.

About the difference between Lazarus release and trunk, you should always use trunk when testing bugs and creating patches. I can recommend trunk, it works well.
« Last Edit: November 02, 2014, 03:42:09 pm by JuhaManninen »
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Graphic changes
« Reply #13 on: November 02, 2014, 03:22:11 pm »
1 - Source code style
  When patching source code (even for my own "trunk"), I always try to respect the original style.
  When too unreadable, I try to JEDI format the code, it doesn't work always, but when it works it gives a good idea of what happens in the code, then put my modifications back to the original file keeping its original format style.

1b - Source code style
  Re : Vincent Snijders in #26885, agree.
  I much prefer the original code because it is easier to put DEBUGGER stop points when the code is unwound line wise in source.

2 - CountBits.
  TFPReaderBMP.CountBits looks a bit strange due to its use of the mod operator my own version would be  :
Code: [Select]
function TFPReaderBMP.CountBits(Value: byte): shortint;
begin
  Result := 0;
  while Value <> 0 do begin
    if (Value and 1) <> 0 then
      Inc(Result);
    Value := Value shr 1;
  end;
end;
  my tests give some improvement (test it) speed-wise. As for Inc(Result); being put on another line, in this case it doesn't make much difference, but in more complicated bits of code, it may be very helpful for debugging.

3 - TWinControl.SendDialogChar
  If I remember well, lagprogramming is probably right on the point of result not being correctly cleared.
  In my personal trunk the code is
Code: [Select]
function TWinControl.SendDialogChar(var Message : TLMKey): Boolean;
var
  ParentForm: TCustomForm;
begin
  ParentForm := GetParentForm(Self);
  Result:=assigned(ParentForm);
  if Result then begin
    Result := ParentForm.DialogChar(Message);
    if Result then
      Message.CharCode := VK_UNKNOWN;
  end;
end;
  Why not compact the conditions ? because when debugging and single stepping it helps to have steps on different lines. (see 1b above). And it is a place in the code where I spent quite a lot of time. ;-)

4 - http://bugs.freepascal.org/view.php?id=25405
  Effectively, Delphi handles it a bit differently.
  The point would be to have a simple demo program showing what problem is caused by the item position in the Controls [] list. In Delphi it was possible to change the non visual create order and that did actually solve some problems with Master/Detail table opening at form/datamodule's creation.

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Graphic changes
« Reply #14 on: November 02, 2014, 04:12:14 pm »
@BrunoK
Regarding source code style each time I analyze, I completely rearrange it. Takes a lot of time because the difference in formatting is huge. After I modify the code, I don't know how to rearrange it backwards. I know how I format my code, I don't understand how original code is formated, mainly because I disagree with some things that I've seen. So, it has to be a very simple function/procedure to modify it and keep the formatting, otherwise there is absolutely no hope in providing any welcome and readable patch.
Regarding 0026885 I understand the point of view of main developers. But it's just a proposal, it's not like I've considered the modification absolutely necessary. They can say: “The change may lead to improvements but debugging that line might become a bottleneck, or the formatting style is not common amongst us, reason why readability is greatly reduced.”.
I got:
“How does your personal code formatting affect patch creation? You changed LCL source and copied it here after all.
If you plan to contribute for Lazarus in future, you must learn to create patches.”
Regarding 4 - http://bugs.freepascal.org/view.php?id=25405
It's one of the reasons why I advertise that bugs may be replaced new ones. It's partially applied but I also wondered about how the tabs should be inserted.

@JuhaManninen
   I can keep track of source file changes easy enough. If other users need a little bit of snappiness, they can try the modified files provided by me freely. It's not a guarantee they will find a solution inside but it's hope. I can easily update the patches, you've seen that already. So, I don't benefit much from current activity of the main developers much. Users that might be interested in using the modified sources are those that have serious problems regarding flickering(frames) and so one. So, regarding Lazarus, few users might benefit from this work, while main developers can't do much for those users. So, when I propose something to you, I consider it might be useful to you. It's just that almost each time main developers backfire the proposals to me. It's not a reason to be upset. It's just that I've tried many times to present you some situations and many times the developers looked bothered by that.
   What's important for me might not be important for somebody else, I accept that. This is the reason why, in order to keep everybody happy and to give back something good to the community I stopped filling reports and started using the forum. This way, users may benefit(partially from the changes because I don't present all of them) and main developers might be interested in some findings that I present. But even this approach appears to bother main developers.
   If you want me to post what I consider abnormal in a different way, you can say so, but  I don't feel like polluting the mailing lists would be better. Maybe you may be interested in what I find within the sources, but others don't.
   Regarding. “Comments from the FPC developers seemed reasonable.”. I understand their point of view but I can't say that I'm happy with the way the main developers “encourage” people. I'm one of those that stopped filling bug reports and I'm sure I'm not the only one. Let's face it, each report filled bothers, reason why each main developer refers to mantis tickets in previous comments of this post. Contrary to main developers, Do-wan Kim provided a patch that(as average) provides 10% speed increase on CPUs in that situation. If the main developers would have closed the ticked we wouldn't have the workaround now. It's a situation where me and the main developers disagreed with benefits for everybody. The same thing might happen here. I understand some of your point of views reason why I stopped presenting my proposals within mantis. But I present them here, I don't keep them hidden.
   So even if we have different opinions, the pascal community is already fragmented enough. Take what's good from my work, if you consider there is any within it, and move on. If you want me to present what I find curious within the sources in a different approach, clearly PROPOSE so, I don't feel bothered. It's just that, regarding my work, I don't know what exactly are main developers interested in, neither the way they prefer to receive the feedback. Maybe you'd consider a new “Lazarus>proposals” entry within Mantis, or a new subject within the forums, I really don't know.

 

TinyPortal © 2005-2018