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

sf::RenderTexture stencil and multisampling support #1285

Merged
merged 1 commit into from Mar 26, 2018

Conversation

binary1248
Copy link
Member

@binary1248 binary1248 commented Sep 17, 2017

SFML Tasks

  • Check reported finding
  • Review code
  • Test implementation

Added support for creation of a stencil attachment and multisampling to sf::RenderTexture. (#1274)

Test with this:

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window(sf::VideoMode(800, 600, 32), "Test", sf::Style::Titlebar | sf::Style::Close);
    window.setVerticalSyncEnabled(true);

    sf::CircleShape circle(200, 9);
    circle.setFillColor(sf::Color::White);
    circle.setPosition(sf::Vector2f(200, 100));

    sf::RenderTexture renderTexture;
    renderTexture.create(800, 600, sf::ContextSettings(0, 0, 8));

    sf::Sprite sprite;
    sprite.setTexture(renderTexture.getTexture());

    bool on = true;

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if ((event.type == sf::Event::Closed) || ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Escape)))
            {
                window.close();
                break;
            }

            if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Space))
            {
                on = !on;
                renderTexture.create(800, 600, sf::ContextSettings(0, 0, on ? 8 : 0));
            }
        }

        renderTexture.clear(sf::Color::Black);
        renderTexture.draw(circle);
        renderTexture.display();

        window.clear();
        window.draw(sprite);
        window.display();
    }
}

@binary1248 binary1248 added this to the 2.5 milestone Sep 17, 2017
@binary1248 binary1248 self-assigned this Sep 17, 2017
@binary1248 binary1248 force-pushed the feature/rendertexture_stencil_multisample branch from d374d67 to 83f614b Compare September 17, 2017 15:17
@binary1248 binary1248 added this to Review & Testing in SFML 2.5.0 Sep 17, 2017
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

My review didn't even work!

@ghost
Copy link

ghost commented Sep 20, 2017

@binary1248 I wish SFML was already pre-compiled with these changes.

@eXpl0it3r
Copy link
Member

See the CI artifact builds

@ghost
Copy link

ghost commented Sep 21, 2017

@eXpl0it3r Thanks. Unfortunately I see nothing. I compiled the PR example, but nothing looks to render. It's all black... I'm using MingW 32..

@ghost
Copy link

ghost commented Sep 21, 2017

@binary1248 Thanks very much for this. It didn't work here :/. It's entirely black and when I press any key, nothing happens too.

if (m_multisampleFrameBuffer)
{
glCheck(GLEXT_glBindFramebuffer(GLEXT_GL_DRAW_FRAMEBUFFER, m_frameBuffer));
glCheck(GLEXT_glBlitFramebuffer(0, 0, m_width, m_height, 0, 0, m_width, m_height, GL_COLOR_BUFFER_BIT, GL_NEAREST));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't GL_LINEAR give smoother results? Or is the parameter irrelevant, because the textures have the same size?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the odd case the driver is stupid, this is faster and imposes no loss of quality.

@Foaly
Copy link
Contributor

Foaly commented Jan 21, 2018

Hi there!
I just tested this PR and it works beautifully for me. Thanks for the work!
One thing I noticed is that when I tried to set the AA level to 16 the example stopped working for me. I checked the return code and saw that the create function failed. Looking into the code I saw that the highest AA level on my computer is 8. I think there should be a notice in sf::err when the creation failed, because of an unsupported AA level, since AA will be the most used case IMO.
I suspect this is also why the example didn't work for @hydroper.

GLint samples = 0;
glGetIntegerv(GLEXT_GL_MAX_SAMPLES, &samples);

if(settings.antialiasingLevel > samples)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest something like this here: err() << "Impossible to create render texture (unsupported anti-aliasing level, maximum supported samples: " << samples << " )" << std::endl;

@eXpl0it3r
Copy link
Member

It's a good point. For the RenderWindow we just fallback to the minimal setting if the config doesn't match and return the difference on sf::err(). Would this be possible here without too much effort or at least to produce an error so it's clear why something failed.

@binary1248
Copy link
Member Author

I'd rather not spam the console with warnings that can be avoided with the necessary precaution. The problem with RenderWindow and this in its current state is that there is no way to check the maximum AA that is supported. It would be simple to do in both cases, but would necessitate adding static functions to both classes.

@Foaly
Copy link
Contributor

Foaly commented Jan 25, 2018

Well I don't know I wouldn't consider it spam, since it informs the user why the creation of the RenderTexture failed. Also pretty much every other error case in create prints the reason to the console. But I agree its a very good idea to also add a function to get the maximum supported AA level.

@eXpl0it3r
Copy link
Member

So you don't want to add any message @binary1248? Can you rebase it as well?

@binary1248 binary1248 force-pushed the feature/rendertexture_stencil_multisample branch 2 times, most recently from 1596508 to f4edff2 Compare March 13, 2018 00:30
@binary1248
Copy link
Member Author

Updated.

@eXpl0it3r eXpl0it3r force-pushed the feature/rendertexture_stencil_multisample branch from f4edff2 to 421e8bb Compare March 26, 2018 21:48
@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Mar 26, 2018
@eXpl0it3r eXpl0it3r merged commit 421e8bb into master Mar 26, 2018
SFML 2.5.0 automation moved this from Ready to Merged / Superseded Mar 26, 2018
@eXpl0it3r eXpl0it3r deleted the feature/rendertexture_stencil_multisample branch March 26, 2018 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

3 participants