-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-09-23 um 10:27 schrieb Riccardo Bortolato:
- HRESULT (__cdecl *volume_created)(struct wined3d_device_parent
*device_parent, void *container_parent, - struct wined3d_volume *volume, void **parent, const struct wined3d_parent_ops **parent_ops); + HRESULT (__cdecl *volume_created)(struct wined3d_device_parent *device_parent, + struct wined3d_texture *texture, unsigned int sub_resource_idx, + void **parent, const struct wined3d_parent_ops **parent_ops);
Is there a reason why you're passing the wined3d_texture container instead of the container parent? It doesn't matter too much because you can either do wined3d_resource_get_parent or d3d*_texture->wined3d_texture, but I think it is a needless change.
It seems the patch is bigger than it needs to be. You could just add unsigned int sub_resource_idx and keep the rest in place. Through struct d3d8_texture *texture inside struct d3d8_volume you already have access to the wined3d texture. With the addition of the sub resource idx you can migrate the volume methods one by one, and in the last patch that removes access to wined3d_volume drop struct wined3d_volume *volume from the callback.
For surfaces in d3d8 and d3d9 you have to make sure you're always creating a d3d8/9_texture struct for plain surfaces, but this is something you can change internally in d3d8/9.
It doesn't matter too much for this patch because the volume API is pretty simple, but the next patch is pretty complicated because it changes all the stuff at once. It'll be terrible to bisect / fix if there are regressions.
I personally prefer going {volume,surface}->wined3d_texture over {volume,surface}->{volume, texture}->wined3d_texture, also I'm not really sure this is applicable to the way ddraw currently works (I might be wrong though).
Ciao, Riccardo
2015-09-23 10:51 GMT+02:00 Stefan Dösinger stefandoesinger@gmail.com:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-09-23 um 10:27 schrieb Riccardo Bortolato:
- HRESULT (__cdecl *volume_created)(struct wined3d_device_parent
*device_parent, void *container_parent, - struct wined3d_volume *volume, void **parent, const struct wined3d_parent_ops **parent_ops); + HRESULT (__cdecl *volume_created)(struct wined3d_device_parent *device_parent, + struct wined3d_texture *texture, unsigned int sub_resource_idx, + void **parent, const struct wined3d_parent_ops **parent_ops);
Is there a reason why you're passing the wined3d_texture container instead of the container parent? It doesn't matter too much because you can either do wined3d_resource_get_parent or d3d*_texture->wined3d_texture, but I think it is a needless change.
It seems the patch is bigger than it needs to be. You could just add unsigned int sub_resource_idx and keep the rest in place. Through struct d3d8_texture *texture inside struct d3d8_volume you already have access to the wined3d texture. With the addition of the sub resource idx you can migrate the volume methods one by one, and in the last patch that removes access to wined3d_volume drop struct wined3d_volume *volume from the callback.
For surfaces in d3d8 and d3d9 you have to make sure you're always creating a d3d8/9_texture struct for plain surfaces, but this is something you can change internally in d3d8/9.
It doesn't matter too much for this patch because the volume API is pretty simple, but the next patch is pretty complicated because it changes all the stuff at once. It'll be terrible to bisect / fix if there are regressions. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2
iQIcBAEBCAAGBQJWAmguAAoJEN0/YqbEcdMwpecP/RqlgzxhIzYQikgOQHhMBuLr 1jOtBW2aAj8qRCPebkM5JAGK+93Y+r9jrkFbJn01ZHKR1fQCfWQteCoMRUIDJ6Iz TUf5IeuKsUMucU6XJ70kh/d8MQCauOKJizvROCg6F1qILF+W6higdGHC/6FjSqqb GiXmYAtqGgOP3BGAjM4W4r8+A7yoqssLhLYmJggn8PlX6yFfdIwl/rpOdRfSC+9T ADrncsPVPpyYFRnQI+atdQuMN5M/GvioqxFS24Fuq65tUqNaXC6D8BsGy0tk6bN2 nZAWkSkrx1bSi24U7RzHbq936Z+qnUp5UH/ZkView8dp5dXiqjmOLucWd4WJ/+0I rMjIduEPuLgDraKnE4vKtn5WpPmV/ITX6If5zI9fTuFBLwyIrC06lIeaGd5VOHiO 2/U00pyIQuS9F6Ir73J0hl5CJITYhmSdT9xsz2sIBbQheZu7OYLqqfIHM0d3DGBk tC3H9X4bJQh9nDzLy4IGVr2W2vqQgKIVGxyVcYFyvA448kP7/W2Z1i+ak9CevFGg fVBrwoPtTzPIoucC9cQ1vnskTITgzSNCuy3t+HBKsbm+0+7VsDnq8CL7wrGbs8uN hSXwpCA4qLsPyK+6XCuITB8rmkGVX3/2j6Ratf2lSgdZVexQfsK/+hWQYIwXzjki otAB34aWZl2+f/socndA =6ndo -----END PGP SIGNATURE-----
On 23 September 2015 at 10:51, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 2015-09-23 um 10:27 schrieb Riccardo Bortolato:
- HRESULT (__cdecl *volume_created)(struct wined3d_device_parent
*device_parent, void *container_parent, - struct wined3d_volume *volume, void **parent, const struct wined3d_parent_ops **parent_ops); + HRESULT (__cdecl *volume_created)(struct wined3d_device_parent *device_parent, + struct wined3d_texture *texture, unsigned int sub_resource_idx, + void **parent, const struct wined3d_parent_ops **parent_ops);
Is there a reason why you're passing the wined3d_texture container instead of the container parent? It doesn't matter too much because you can either do wined3d_resource_get_parent or d3d*_texture->wined3d_texture, but I think it is a needless change.
Not all d3d9 surfaces will necessarily have a corresponding d3d9 texture, and I'm not sure it's worth changing that. So in that regard this is fine. (But please fix the '*' placement.) Once you pass a wined3d_texture the container_parent parameter becomes redundant.
It seems the patch is bigger than it needs to be. You could just add unsigned int sub_resource_idx and keep the rest in place. Through struct d3d8_texture *texture inside struct d3d8_volume you already have access to the wined3d texture. With the addition of the sub resource idx you can migrate the volume methods one by one, and in the last patch that removes access to wined3d_volume drop struct wined3d_volume *volume from the callback.
Yeah, this. Patches should only do a single thing as much as possibly/reasonable. It's a shame you can't drop the volume/surface parameter from the callback in the first patch, but not so much of an issue. In fact, you don't even necessarily need to store the texture/sub_resource_idx everywhere in the first patch. You should probably start with introducing something like wined3d_texture_map() and using it for d3d11. Then the second patch could e.g. be to use wined3d_texture_map() in d3d9 as well, with the callback interface change being a logical consequence of that.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-10-01 um 17:42 schrieb Henri Verbeet:
Not all d3d9 surfaces will necessarily have a corresponding d3d9 texture, and I'm not sure it's worth changing that. So in that regard this is fine. (But please fix the '*' placement.) Once you pass a wined3d_texture the container_parent parameter becomes redundant.
Just to make sure I understand this right, especially in combination with https://www.winehq.org/pipermail/wine-devel/2015-September/109145.html : There may not always be a d3d9_texture for a d3d9_surface but there's always a wined3d_texture, which we store in the d3d9_surface instead of the d3d9_texture. If we need the d3d9_texture for some reason we get the parent. I.e. pretty much what Riccardo has done in this regard in this patch?
On 1 October 2015 at 17:50, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 2015-10-01 um 17:42 schrieb Henri Verbeet:
Not all d3d9 surfaces will necessarily have a corresponding d3d9 texture, and I'm not sure it's worth changing that. So in that regard this is fine. (But please fix the '*' placement.) Once you pass a wined3d_texture the container_parent parameter becomes redundant.
Just to make sure I understand this right, especially in combination with https://www.winehq.org/pipermail/wine-devel/2015-September/109145.html : There may not always be a d3d9_texture for a d3d9_surface but there's always a wined3d_texture, which we store in the d3d9_surface instead of the d3d9_texture. If we need the d3d9_texture for some reason we get the parent. I.e. pretty much what Riccardo has done in this regard in this patch?
Yeah, although it's not necessarily wrong to just store the d3d9 texture pointer in the d3d9 surface as well.
-
Henri Verbeet -
Riccardo Bortolato -
Stefan Dösinger