Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for scissor testing. #1451

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Added support for scissor testing. #1451

merged 1 commit into from
Nov 20, 2023

Conversation

binary1248
Copy link
Member

@binary1248 binary1248 commented Jun 10, 2018

This is a subset of #846 and implements a part of #1.

Test with:

#include <SFML/Graphics.hpp>

int main()
{
    auto window = sf::RenderWindow{sf::VideoMode{{200u, 200u}}, "Test"};

    auto shape = sf::CircleShape{100.f};
    shape.setFillColor(sf::Color::Green);

    auto left = window.getView();
    left.setScissor(sf::FloatRect{{0.f, 0.f}, {0.5f, 1.f}});

    auto right = window.getView();
    right.setScissor(sf::FloatRect{{0.5f, 0.f}, {0.5f, 1.f}});

    while (window.isOpen())
    {
        for (auto event = sf::Event{}; window.pollEvent(event);)
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.setView(window.getDefaultView());
        window.clear();

        window.setView(left);
        window.clear(sf::Color::Red);

        window.setView(right);
        window.draw(shape);

        window.display();
    }
}

@binary1248 binary1248 self-assigned this Jun 10, 2018
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Aug 13, 2018
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Aug 13, 2018
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Aug 13, 2018
@eXpl0it3r eXpl0it3r removed this from Review & Testing in SFML 2.6.0 May 26, 2020
@eXpl0it3r eXpl0it3r removed this from the 2.6 milestone May 26, 2020
@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #1451 (283c636) into master (c1f92ed) will increase coverage by 0.06%.
The diff coverage is 48.21%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
+ Coverage   39.28%   39.35%   +0.06%     
==========================================
  Files         236      236              
  Lines       20763    20816      +53     
  Branches     5151     5179      +28     
==========================================
+ Hits         8157     8192      +35     
- Misses      11785    11804      +19     
+ Partials      821      820       -1     
Files Coverage Δ
include/SFML/Graphics/RenderTarget.hpp 66.66% <ø> (ø)
include/SFML/Graphics/View.hpp 80.00% <100.00%> (+5.00%) ⬆️
src/SFML/Graphics/View.cpp 100.00% <100.00%> (ø)
src/SFML/Graphics/Texture.cpp 73.18% <66.66%> (-0.03%) ⬇️
src/SFML/Graphics/RenderTextureImplFBO.cpp 34.45% <0.00%> (-0.80%) ⬇️
src/SFML/Graphics/RenderTarget.cpp 30.39% <27.58%> (+1.93%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1f92ed...283c636. Read the comment docs.

@binary1248
Copy link
Member Author

Updated to current master.

@binary1248
Copy link
Member Author

Updated to current master and added tests.

Copy link
Member

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few items you should consider but besides that I'm cool with this PR 👍

include/SFML/Graphics/RenderTarget.hpp Outdated Show resolved Hide resolved
src/SFML/Graphics/RenderTarget.cpp Outdated Show resolved Hide resolved
test/Graphics/View.test.cpp Show resolved Hide resolved
src/SFML/Graphics/RenderTarget.cpp Outdated Show resolved Hide resolved
@binary1248
Copy link
Member Author

Fixed.

@binary1248
Copy link
Member Author

Updated.

@ChrisThrasher ChrisThrasher requested review from MarioLiebisch, kimci86 and Bromeon and removed request for kimci86 June 4, 2023 22:49
@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Jun 18, 2023
Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

Some small, mostly documentation comments.
Plus, we may want to document somewhere, what the difference between viewport and scissor testing is.

include/SFML/Graphics/View.hpp Outdated Show resolved Hide resolved
src/SFML/Graphics/RenderTarget.cpp Show resolved Hide resolved
src/SFML/Graphics/RenderTarget.cpp Outdated Show resolved Hide resolved
src/SFML/Graphics/RenderTarget.cpp Show resolved Hide resolved
@binary1248
Copy link
Member Author

Updated.

Plus, we may want to document somewhere, what the difference between viewport and scissor testing is.

Added a paragraph to the sf::View documentation regarding this issue.

@kimci86
Copy link
Contributor

kimci86 commented Jul 8, 2023

It doesn't work well in interaction with sf::Font. EDIT: It is now fixed.

See the result of the following code:

#include <SFML/Graphics.hpp>

int main()
{
    auto target = sf::RenderTexture{};
    if (!target.create({400, 300}))
        return 1;

    auto font = sf::Font{};
    // https://github.com/SFML/SFML/raw/master/test/Graphics/tuffy.ttf
    if (!font.loadFromFile("tuffy.ttf"))
        return 1;

    auto text = sf::Text{font, "0123456789ABCDEFGHIJKLMNOPQ", 30};
    text.setFillColor(sf::Color::White);
    text.setPosition({0.f, 100.f});

    target.clear(sf::Color{64, 64, 64});

    auto scissorView = target.getDefaultView();
    scissorView.setScissor({{0.1f, 0.1f}, {0.8f, 0.8f}});
    target.setView(scissorView);
    target.clear(sf::Color{128, 192, 128});
    target.draw(text);

    target.display();

    if (!target.getTexture().copyToImage().saveToFile("result.png"))
        return 1;

    return 0;
}

result

Below is the expected result, which I get after applying this patch (quick hack, probably not a proper solution).

diff --git a/src/SFML/Graphics/Texture.cpp b/src/SFML/Graphics/Texture.cpp
index 7c6df2e5..70b5286d 100644
--- a/src/SFML/Graphics/Texture.cpp
+++ b/src/SFML/Graphics/Texture.cpp
@@ -463,6 +463,8 @@ void Texture::update(const std::uint8_t* pixels, const Vector2u& size, const Vec
         // Make sure that the current texture binding will be preserved
         const priv::TextureSaver save;
 
+        glCheck(glDisable(GL_SCISSOR_TEST));
+
         // Copy pixels from the given array to the texture
         glCheck(glBindTexture(GL_TEXTURE_2D, m_texture));
         glCheck(glTexSubImage2D(GL_TEXTURE_2D,

result

@binary1248
Copy link
Member Author

Updated. Turns out scissor testing was having an affect on our texture copying when the FBO blit implementation is used.

src/SFML/Graphics/View.cpp Show resolved Hide resolved
@binary1248
Copy link
Member Author

Updated.

Copy link
Contributor

@kimci86 kimci86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm the problem with font rendering is not there anymore. Looks good to me besides minor comments below.

src/SFML/Graphics/RenderTarget.cpp Outdated Show resolved Hide resolved
src/SFML/Graphics/View.cpp Outdated Show resolved Hide resolved
@binary1248
Copy link
Member Author

Updated.

@ChrisThrasher ChrisThrasher merged commit 5a2f30c into master Nov 20, 2023
190 checks passed
@ChrisThrasher ChrisThrasher deleted the feature/scissor branch November 20, 2023 18:01
@eXpl0it3r eXpl0it3r mentioned this pull request May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants