Re: [PATCH 1/4] wined3d: Vertex fog uses the absolute eye position z (v3).
Am Donnerstag, 27. November 2014 schrieb Stefan Dösinger:
This is written from scratch, but inspired by the patch from Joachim Priesner. The included test tests the absolute minimum necessary to show that the abs() needs to be placed in the fixed function vertex pipeline replacement shader. It ignores vertex and pixel shader behavior.
Does this mean I can finally stop worrying about my version of the bug fix and concentrate on getting my other two pending patch sets in? If so, I'd very much like feedback on 107283 Pending Joachim Priesner ws2_32: WS_get_local_ips: Fix ordering of IP addresses by metric if two addresses have the same metric (try 2) which seems pretty straightforward and fixes a real bug, and also on [PATCH 3/3] comctl32: Implement the TaskDialog function (try 2) [PATCH 2/3] comctl32: Basic implementation of TaskDialogIndirect (try 2) [PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file (try 2) which I submitted almost two months ago, but which, after one round of feedback, vanished from the patches list without further comment. What can I do to make these patches better? Add tests? Split into smaller files? Give up altogether? NB, both patch sets fix issues with applications I work with -- does creating a bug report increase the chances of the patches getting looked at? Thanks for any help. Joachim
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-11-27 14:14, schrieb Joachim Priesner:
Am Donnerstag, 27. November 2014 schrieb Stefan Dösinger:
This is written from scratch, but inspired by the patch from Joachim Priesner. The included test tests the absolute minimum necessary to show that the abs() needs to be placed in the fixed function vertex pipeline replacement shader. It ignores vertex and pixel shader behavior.
Does this mean I can finally stop worrying about my version of the bug fix and concentrate on getting my other two pending patch sets in?
Yes. Version 3 of my patches made it in, so your game should work now. The part from your test that tested the vertex shader behavior is still on my todo list, but since we don't know any application that has a problem with the current behavior it is fairly low priority. I must admit that the way we handled this was not our finest hour. It is quite clear that we have a big problem with handling new or sporadic contributors.
ws2_32: WS_get_local_ips: Fix ordering of IP addresses by metric if two addresses have the same metric (try 2)
which seems pretty straightforward and fixes a real bug, and also on I agree that it is straightforward. I only see a minor issue: I think you can drop the if (!left || !right) check - the qsort man page suggests this is not necessary.
I guess writing tests for this is impossible because the input comes directly from the host system's network setup which can only be modified with root permissions. The other problem ws2_32 has is that there's apparently no one except Alexandre who claims to know it well.
[PATCH 3/3] comctl32: Implement the TaskDialog function (try 2) [PATCH 2/3] comctl32: Basic implementation of TaskDialogIndirect (try 2) [PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file (try 2) I have never touched comctl32, so I'm not really qualified here. The overall feeling is that the code looks like is is copypasted together from various places. (The style inconsistencies trigger that suspicion). Combined with the lack of tests it raises the question of how solid this implementation is in the long run.
Also the defines like TD_WARNING_ICON look like they're copypasted from Microsoft's header. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUeERNAAoJEN0/YqbEcdMwCIsP/0GAOAbqlEQRR0Md7dHE6z2n 3tHm8Too/wkPSmF89r0/PwPAn9eEuoWxpwVFSOsup8fyFAGZ66BEOITnQF9hiE4A SPtkrkV4HNNgw/m+iYZuUmf4daZLE8/iEwVenaaDrwpruFC84zGBrKBOfpNba8fH JeM1peJftvh9uR8aR9ARIonGKbghIsby0ce5SbKMSZJ2ufycsAhx+YQOyXmm6G4A oKADQDmbdtfMopeswyZyInKBiOOFCR+XrnbRLxMl/cj4u9gT7HisTIQsNzYoDdym qT9XSRaI/ksbOW2siEbKkKhxLr+27WbJRuKL9tVN8fydbj65+KDQDeMLHQe+1DTO kOU2/9l3IDXICDP3x7EtlK1sO3CdcaTrWyqz6BUInPVmoxtwVM7qWebpw8Ulq0o4 AAU5r1Y7A1tD8iAB1q15iw66kGQDDdaOtaZisbJHzznpWkL1MI10PVk2qMpONOaE Al6XauEZaiJR+0Fi/cqt9LOV+iEg+0OeU/cOz+DPR2FhkeDhec06No/EFqbNusSK klYfF7SqQ4OwZs3LcQBQwYlQy1ienj9oenX0KxlCxAXahTa1z3DGDCuuji9pPUnF 9fZc9Aaa1hFP7sWf2WpgVqyB5/HFZUWm2uyDVA/Ft+AJyld0046K0jPVowb0CXCS auBFcZHd1dZRCIPIjgN4 =+m+F -----END PGP SIGNATURE-----
I really appreciate you giving me feedback, thanks! (Your guidance on the abs() patch was very helpful, too.)
[PATCH 3/3] comctl32: Implement the TaskDialog function (try 2) [PATCH 2/3] comctl32: Basic implementation of TaskDialogIndirect (try 2) [PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file (try 2) I have never touched comctl32, so I'm not really qualified here. The overall feeling is that the code looks like is is copypasted together from various places. (The style inconsistencies trigger that suspicion). Combined with the lack of tests it raises the question of how solid this implementation is in the long run.
Yes, this is mostly derived from MessageBoxW. I'll try to make this more consistent and add more tests as far as it is possible.
Also the defines like TD_WARNING_ICON look like they're copypasted from Microsoft's header.
What would be the way to define them properly? Best Joachim
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-11-28 11:35, schrieb Joachim Priesner:
Yes, this is mostly derived from MessageBoxW.
I'll try to make this more consistent and add more tests as far as it is possible. What would work better IMHO is splitting them up in multiple steps to build the new dialog step by step, each step accompanied with tests. E.g. start with testing handling of invalid input like pTaskConfig == NULL. Next, replace the current stub with its own dialog with equal functionality. After that, add support for the icon. And so on.
The way I've added additional tests to test_negative_fixedfunction_fog is an example of what I mean. (I have not done the same with the implementation because unconditionally adding the abs and then removing it again for pretransformed geometry would have introduced a temporary regression.)
Also the defines like TD_WARNING_ICON look like they're copypasted from Microsoft's header. What would be the way to define them properly? This topic has come up on this list before. Unfortunately I couldn't find a good answer in the archives with a quick Google search :-(
The rough answer: Add things exactly when you need them (or with a dedicated header changing patch right before that). Usually our headers deliberately use a different code style than Microsoft's headers to show that they are not copypasted. The header has to be compatible from a compiler point of view, but depending on what you define you have some freedom to write it down in a different way. With simple #defines that freedom is limited though. The facts recorded in the header are not copyrightable. The specific incarnation of the header may be copyrightable though, so we're really careful here. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUe1/lAAoJEN0/YqbEcdMwKS0P/3VifCpni+l3QV8CySis+Hwp DZQ3K07AbfiL+YN5jt1cPua93arczUUa7MakO4ff7x7QPjhZ2PZ1hlmYdBjGk9qH GvEX1favCWTsRIURP3e8EJ1/Iet0BiYEj9+NWKQ62t8L/zV/iosoBVRg+iGEj3cX PTVaX5EAMdOkPiU124otEamtg8RGVmTaC/KixqlWdXEPtiNqu1scC0LFTh/4x0nk QHedD+CCrqIN6at8bl45gbUQma45UJ5sbaC81xaPM3DGp5WPhuN22CdofkI4iwfN 2lqFzG2+8vQ2f7p49L1QTDqX/C3CqIkPvPQAQylDd2SnKW7bvZClTv+UV5slom+T Cg5h9IwkjSe/cVLjW2MaQfWeX/W/QsHq8Len80cFQ592hKydARUcXl9Gr9iw95kb j6RsA/fFHJLltll0/OLC2m9byZztyCieaGfIzbxO3TDA0QcPycvb3zCPbuFO3nlc 9psCCpsLHZaxV/Y6SC655oNWk5jF+IWUytVcANFR29g0cU3C+icEVU6u2YEGmONK S81keqtX/lNWl5+IaxYXNT48/TPD0CwJP7UGyciDpA0VDJgcVu4cP3U5tfODmGg7 wV8TiDTW8sF70zm+7xXLLo9RrpCXmK/iN28Itg4moq8i6QrwAPQzAe/gDISHjHAh Q+aXkgmzn04EvIF9pLHK =QwHI -----END PGP SIGNATURE-----
participants (2)
-
Joachim Priesner -
Stefan Dösinger