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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some more OpenGL stuff #867

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@binary1248
Member

binary1248 commented Apr 16, 2015

This pull request adds:

  • Vendor and renderer strings to sf::ContextSettings so that the application developer can check them if they need to
  • Adds an additional check for "Microsoft Corporation" and "GDI Generic" strings, just because Microsoft loves OpenGL 馃槒
  • Adds NvOptimusEnablement export (as described here) to sfml-main so that any application that links to it will export that symbol and tell the Nvidia driver to run the application using the discrete GPU (sfml-main is always static, so the symbol will always be exported by the application if it gets linked in)

This is kind of a knee-jerk response to all those threads that have been popping up on the forum recently that are 99% caused by users not knowing that they are running on the GDI implementation on Windows. With this, we can leave it up to the application developer to sort out their users' problems and not come to us straight away every time something mysteriously breaks.

It would be nice if this could get in for 2.3. It would save us (especially me) a lot of forum headaches 馃榿.

@LaurentGomila, @mantognini, @eXpl0it3r, @TankOs, @Bromeon, @MarioLiebisch, @Sonkun feel free to review and provide prompt feedback 馃槈.

@binary1248 binary1248 self-assigned this Apr 16, 2015

@binary1248 binary1248 added this to the 2.3 milestone Apr 16, 2015

@@ -76,6 +81,8 @@ struct ContextSettings
unsigned int majorVersion; ///< Major number of the context version to create
unsigned int minorVersion; ///< Minor number of the context version to create
Uint32 attributeFlags; ///< The attribute flags to create the context with
std::string vendorName; ///< Name of the vendor of the OpenGL implementation
std::string rendererName; ///< Name of the OpenGL renderer implementation

This comment has been minimized.

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

Although this is an obvious place for adding these members, it's strange because you can set them but this would have no effect, they are just relevant when you read the context settings back from the window.

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

Although this is an obvious place for adding these members, it's strange because you can set them but this would have no effect, they are just relevant when you read the context settings back from the window.

This comment has been minimized.

@binary1248

binary1248 Apr 16, 2015

Member

When you default construct an sf::ContextSettings and only modify 1 field, you get the same effect. The user shouldn't be surprised by stuff being overwritten with the actual values when context creation is done.

@binary1248

binary1248 Apr 16, 2015

Member

When you default construct an sf::ContextSettings and only modify 1 field, you get the same effect. The user shouldn't be surprised by stuff being overwritten with the actual values when context creation is done.

This comment has been minimized.

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

What I mean is that it gives the impression that you can set them, to select the GPU (in case of dual chip) and the renderer (software/hardware, ...).

ContextSettings settings;
settings.vendorName = "...";
settings.rendererName = "...";
window.create(..., settings);

This is not the same as the other members, which really select some context features -- just with the predefined default value if you don't touch them.

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

What I mean is that it gives the impression that you can set them, to select the GPU (in case of dual chip) and the renderer (software/hardware, ...).

ContextSettings settings;
settings.vendorName = "...";
settings.rendererName = "...";
window.create(..., settings);

This is not the same as the other members, which really select some context features -- just with the predefined default value if you don't touch them.

This comment has been minimized.

@binary1248

binary1248 Apr 16, 2015

Member

I wouldn't use the word "select" 馃槈. They are more like "hints" for SFML to select a configuration that best suits the requested settings.

If you want, I can add a note to the documentation that states that these fields are not taken into consideration when creating the context.

@binary1248

binary1248 Apr 16, 2015

Member

I wouldn't use the word "select" 馃槈. They are more like "hints" for SFML to select a configuration that best suits the requested settings.

If you want, I can add a note to the documentation that states that these fields are not taken into consideration when creating the context.

This comment has been minimized.

@Bromeon

Bromeon Apr 16, 2015

Member

I agree with Laurent, also the name "ContextSetting" implies that it's something the user can set/configure.

One alternative would be a new type like sf::Joystick::Identification, in addition to a sf::Window::getDriverIdentification() method or similar. Other ideas?

@Bromeon

Bromeon Apr 16, 2015

Member

I agree with Laurent, also the name "ContextSetting" implies that it's something the user can set/configure.

One alternative would be a new type like sf::Joystick::Identification, in addition to a sf::Window::getDriverIdentification() method or similar. Other ideas?

This comment has been minimized.

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

What can a developer do with these extra information, other than output a warning, which we already do? In other words, is it really useful to expose it in the public API?

@LaurentGomila

LaurentGomila Apr 16, 2015

Member

What can a developer do with these extra information, other than output a warning, which we already do? In other words, is it really useful to expose it in the public API?

This comment has been minimized.

@mantognini

mantognini Apr 16, 2015

Member

To continue on what Bromeon said, I see this as an introspection fonction and as such should go along with functions to get the number of cpu and stuff like that. I feel it would be wiser to design this larger API first. I suggest therefore that we don't consider this feature for 2.3 in order not to stall its release.

@mantognini

mantognini Apr 16, 2015

Member

To continue on what Bromeon said, I see this as an introspection fonction and as such should go along with functions to get the number of cpu and stuff like that. I feel it would be wiser to design this larger API first. I suggest therefore that we don't consider this feature for 2.3 in order not to stall its release.

This comment has been minimized.

@binary1248

binary1248 Apr 16, 2015

Member

I agree with Laurent, also the name "ContextSetting" implies that it's something the user can set/configure.

One alternative would be a new type like sf::Joystick::Identification, in addition to a sf::Window::getDriverIdentification() method or similar. Other ideas?

If the being a setting really implies being able to be set by the user then updating it with the "correct" data after a context is created doesn't make sense either. I could make the same argument and ask why not split all context information off into a separate data structure?

What can a developer do with these extra information, other than output a warning, which we already do? In other words, is it really useful to expose it in the public API?

This is as useful/unuseful to expose as the context version, and we expose that already. There are many things a developer can do with this information, one of the most obvious ones being selecting a higher level of AA than 8, which translates to one of Nvidia's "special" AAs such as TXAA or FXAA. There are also other non-trivial differences between AMD and Nvidia OpenGL implementations that might hint the developer to optimize their rendering in different ways.

To continue on what Bromeon said, I see this as an introspection fonction and as such should go along with functions to get the number of cpu and stuff like that. I feel it would be wiser to design this larger API first. I suggest therefore that we don't consider this feature for 2.3 in order not to stall its release.

Even if this is part of a wider API, where else would it be? In the system module? The introspection functions don't have to all be added at the same time, one can even consider the context version to be a limited form of introspection already.

The main reason I think this should be coupled with the 2.3 release is the fact that we have made it too easy to get away with a software context on Windows. The application would simply crash before, and at that point the developer might have asked for hardware/driver information. Now, it won't crash but simply exhibit unexpected behaviour which might not make the actual problem that obvious any more. If we don't provide them with a non-OpenGL way of getting this information they simply won't bother to check and instead come running to us with "bug reports" after 2.3 is released, kind of like what is already happening now to a limited degree.

Outright crashing when a software context was created before was just broken, which is why I fixed it. This is just the second half of the solution, and I'd prefer not splitting this into a separate release if possible.

@binary1248

binary1248 Apr 16, 2015

Member

I agree with Laurent, also the name "ContextSetting" implies that it's something the user can set/configure.

One alternative would be a new type like sf::Joystick::Identification, in addition to a sf::Window::getDriverIdentification() method or similar. Other ideas?

If the being a setting really implies being able to be set by the user then updating it with the "correct" data after a context is created doesn't make sense either. I could make the same argument and ask why not split all context information off into a separate data structure?

What can a developer do with these extra information, other than output a warning, which we already do? In other words, is it really useful to expose it in the public API?

This is as useful/unuseful to expose as the context version, and we expose that already. There are many things a developer can do with this information, one of the most obvious ones being selecting a higher level of AA than 8, which translates to one of Nvidia's "special" AAs such as TXAA or FXAA. There are also other non-trivial differences between AMD and Nvidia OpenGL implementations that might hint the developer to optimize their rendering in different ways.

To continue on what Bromeon said, I see this as an introspection fonction and as such should go along with functions to get the number of cpu and stuff like that. I feel it would be wiser to design this larger API first. I suggest therefore that we don't consider this feature for 2.3 in order not to stall its release.

Even if this is part of a wider API, where else would it be? In the system module? The introspection functions don't have to all be added at the same time, one can even consider the context version to be a limited form of introspection already.

The main reason I think this should be coupled with the 2.3 release is the fact that we have made it too easy to get away with a software context on Windows. The application would simply crash before, and at that point the developer might have asked for hardware/driver information. Now, it won't crash but simply exhibit unexpected behaviour which might not make the actual problem that obvious any more. If we don't provide them with a non-OpenGL way of getting this information they simply won't bother to check and instead come running to us with "bug reports" after 2.3 is released, kind of like what is already happening now to a limited degree.

Outright crashing when a software context was created before was just broken, which is why I fixed it. This is just the second half of the solution, and I'd prefer not splitting this into a separate release if possible.

This comment has been minimized.

@Havoc2

Havoc2 Apr 18, 2015

I don't really know the codebase of SFML or your coding conventions, but here's an idea (I haven't tested this idea, though):

Make vendorName and rendererName private variables in the ContextSettings struct and GlContext a friend class of the ContextSettings struct. Then add getter methods for vendorName and rendererName in the ContextSettings struct, so application developers can explicitly get the values of vendorName and rendererName by calling these methods.

Note: You'd probably have a problem with the friend class, because GlContext is in a different namespace, so you'd probably also need to add a forward declaration of the GlContext class before the definition of the ContextSettings struct.

@Havoc2

Havoc2 Apr 18, 2015

I don't really know the codebase of SFML or your coding conventions, but here's an idea (I haven't tested this idea, though):

Make vendorName and rendererName private variables in the ContextSettings struct and GlContext a friend class of the ContextSettings struct. Then add getter methods for vendorName and rendererName in the ContextSettings struct, so application developers can explicitly get the values of vendorName and rendererName by calling these methods.

Note: You'd probably have a problem with the friend class, because GlContext is in a different namespace, so you'd probably also need to add a forward declaration of the GlContext class before the definition of the ContextSettings struct.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 16, 2015

Member

Added AmdPowerXpressRequestHighPerformance as described in this thread.

Member

binary1248 commented Apr 16, 2015

Added AmdPowerXpressRequestHighPerformance as described in this thread.

Added OpenGL vendor and renderer strings to sf::ContextSettings for e鈥
鈥sy access and avoiding the need to write OpenGL code in the application, added NvOptimusEnablement and AmdPowerXpressRequestHighPerformance exports to sfml-main to inform the driver that the SFML application should be run using the discrete GPU in a multi-GPU environment.

This was referenced Apr 18, 2015

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 18, 2015

Member

Superseded by #869 and #870.

Member

binary1248 commented Apr 18, 2015

Superseded by #869 and #870.

@binary1248 binary1248 closed this Apr 18, 2015

@binary1248 binary1248 removed this from the 2.3 milestone Apr 18, 2015

@binary1248 binary1248 removed their assignment Apr 18, 2015

@binary1248 binary1248 deleted the feature/renderer_information branch Apr 18, 2015

@mantognini mantognini added this to the 2.4 milestone Aug 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment