{"action":"created","issue":{"url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/2688","repository_url":"https://api.github.com/repos/MovingBlocks/Terasology","labels_url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/2688/labels{/name}","comments_url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/2688/comments","events_url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/2688/events","html_url":"https://github.com/MovingBlocks/Terasology/issues/2688","id":196805251,"number":2688,"title":"SetViewportToSizeOf class equality should be dependent on width/height, not fbo name","user":{"login":"emanuele3d","id":6934584,"avatar_url":"https://avatars.githubusercontent.com/u/6934584?v=3","gravatar_id":"","url":"https://api.github.com/users/emanuele3d","html_url":"https://github.com/emanuele3d","followers_url":"https://api.github.com/users/emanuele3d/followers","following_url":"https://api.github.com/users/emanuele3d/following{/other_user}","gists_url":"https://api.github.com/users/emanuele3d/gists{/gist_id}","starred_url":"https://api.github.com/users/emanuele3d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emanuele3d/subscriptions","organizations_url":"https://api.github.com/users/emanuele3d/orgs","repos_url":"https://api.github.com/users/emanuele3d/repos","events_url":"https://api.github.com/users/emanuele3d/events{/privacy}","received_events_url":"https://api.github.com/users/emanuele3d/received_events","type":"User","site_admin":false},"labels":[{"id":56016179,"url":"https://api.github.com/repos/MovingBlocks/Terasology/labels/3D-Wizardry","name":"3D-Wizardry","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":{"login":"emanuele3d","id":6934584,"avatar_url":"https://avatars.githubusercontent.com/u/6934584?v=3","gravatar_id":"","url":"https://api.github.com/users/emanuele3d","html_url":"https://github.com/emanuele3d","followers_url":"https://api.github.com/users/emanuele3d/followers","following_url":"https://api.github.com/users/emanuele3d/following{/other_user}","gists_url":"https://api.github.com/users/emanuele3d/gists{/gist_id}","starred_url":"https://api.github.com/users/emanuele3d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emanuele3d/subscriptions","organizations_url":"https://api.github.com/users/emanuele3d/orgs","repos_url":"https://api.github.com/users/emanuele3d/repos","events_url":"https://api.github.com/users/emanuele3d/events{/privacy}","received_events_url":"https://api.github.com/users/emanuele3d/received_events","type":"User","site_admin":false},"assignees":[{"login":"emanuele3d","id":6934584,"avatar_url":"https://avatars.githubusercontent.com/u/6934584?v=3","gravatar_id":"","url":"https://api.github.com/users/emanuele3d","html_url":"https://github.com/emanuele3d","followers_url":"https://api.github.com/users/emanuele3d/followers","following_url":"https://api.github.com/users/emanuele3d/following{/other_user}","gists_url":"https://api.github.com/users/emanuele3d/gists{/gist_id}","starred_url":"https://api.github.com/users/emanuele3d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emanuele3d/subscriptions","organizations_url":"https://api.github.com/users/emanuele3d/orgs","repos_url":"https://api.github.com/users/emanuele3d/repos","events_url":"https://api.github.com/users/emanuele3d/events{/privacy}","received_events_url":"https://api.github.com/users/emanuele3d/received_events","type":"User","site_admin":false}],"milestone":null,"comments":8,"created_at":"2016-12-20T23:11:46Z","updated_at":"2016-12-25T23:26:56Z","closed_at":null,"body":"### What you were trying to do\r\nEnsure glViewport is called only if necessary.\r\n\r\n### What actually happened\r\nIt gets called whenever the FBO changes from one node to the next. Should change only when the width/height dimensions of the FBOs in consecutive nodes change.\r\n\r\n### How to reproduce\r\n- Run Terasology, start a game, exit terasology.\r\n- Check log for RENDER TASK LIST: consecutive nodes should have SetViewportToSizeOfTask lines with different width/height values, or the following nodes should have no SetViewportToSizeOfTask line at all.\r\n\r\n### Cause\r\nThis is due to the SetViewportToSizeOf class comparing instances based on the name of the FBO they get the dimensions from. Instead, they should compare the width and height values. Methods hashCode and equals must be refactored accordingly. Obtaining the fbo dimensions is the architectural challenge.\r\n\r\nIt shouldn't be too difficult but must be handled carefully.\r\n"},"comment":{"url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/comments/269141576","html_url":"https://github.com/MovingBlocks/Terasology/issues/2688#issuecomment-269141576","issue_url":"https://api.github.com/repos/MovingBlocks/Terasology/issues/2688","id":269141576,"user":{"login":"emanuele3d","id":6934584,"avatar_url":"https://avatars.githubusercontent.com/u/6934584?v=3","gravatar_id":"","url":"https://api.github.com/users/emanuele3d","html_url":"https://github.com/emanuele3d","followers_url":"https://api.github.com/users/emanuele3d/followers","following_url":"https://api.github.com/users/emanuele3d/following{/other_user}","gists_url":"https://api.github.com/users/emanuele3d/gists{/gist_id}","starred_url":"https://api.github.com/users/emanuele3d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emanuele3d/subscriptions","organizations_url":"https://api.github.com/users/emanuele3d/orgs","repos_url":"https://api.github.com/users/emanuele3d/repos","events_url":"https://api.github.com/users/emanuele3d/events{/privacy}","received_events_url":"https://api.github.com/users/emanuele3d/received_events","type":"User","site_admin":false},"created_at":"2016-12-25T23:26:56Z","updated_at":"2016-12-25T23:26:56Z","body":"@eviltak: thank you for your analyses and for the example you make, it certainly makes your thinking very clear. \r\n\r\nOn the first issue. I agree that in your example state changes 1 to 4 are redundant given that they all match the default. However, the error is not in the reset state changes but in using instances of SetViewportToSizeOf that set the viewport to default values. Those nodes should not have a SetViewportToSizeOf instance among their desired state changes. This is the cause of those redundant state changes and this what needs to be addressed: desired state changes should only be allowed to have non-default values. By focusing on the reset you are focusing on the symptoms, not the cause. I want to get the cause addressed and it deserves a separate issue.\r\n\r\nOn the second issue:\r\n\r\n> This is because SetViewportToSizeOf is the only StateChange whose instances have a possibility of being value equal. No other StateChange has any two instances which are value equal (not considering the state resets).\r\n\r\nPerhaps I'm not understanding what you mean here. Many, if not all, state changes have a possibility of being value equal. Take for example the BindFBO state change. Nodes BufferClearingNode, BackdropReflectionNode, WorldReflectionNode all use it to request binding to the same non-default FBO: sceneReflected. The TaskListGenerator correctly adds a BindFBOTask when iterating over the first node, but does not for the next two because it found the desired state changes to be value-equal to the persistent one. So, I'm not sure what you mean here?\r\n\r\n> Furthermore, no other StateChange has non-reset instances which are value identical to the reset instances with or without a node which does not contain a StateChange of the same type in between them.\r\n\r\nAs I mentioned above this isn't meant to happen. If you are a module developer and you make a new node you should know that it doesn't make sense to add a SetViewportToSizeOf state change with dimensions that are the same as the gbuffer, as that should be [1] the default. Right now I'm still in a mopping-up phase after the GSOC. I feel that these issues with FBOs and viewports will be slowly addressed by my cleanup. But ultimately non-reset StateChange instances should always have non-default values. And that's what we need to address.\r\n\r\n[1] Thanks to this discussion I realized that the dimensions of the gbuffer are not the default: the dimensions of the display are. In  normal circumstances this is not a problem but when taking a screenshot the display dimensions can be different from the dimensions of the gbuffer. Another issue to address."}}
{"actor":{"display_login":"emanuele3d"}}