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

glm transition - deprecations implicit conversions, remove implicit conversions from the core. #5594

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

bakercp
Copy link
Member

@bakercp bakercp commented May 5, 2017

This is related to #5440, the effort to ease the glm transition.

I looked into deprecating entire classes (e.g. ofVec2f, etc), but it created a huge mess of deprecation warnings, so instead I opted to just add deprecation warnings in those places where glm/of conversions were happening implicitly.

I also discovered a few remaining places where ofVec* was being used explicitly and a few interesting places in ofNode where when adding an glm::vec3 + glm::vec4 resulted in an intermediate conversion to ofVec3f.

Currently this shouldn't produce any deprecations warnings from the core itself, but only with non-conforming examples and user code that still employs ofVec*.

@@ -597,7 +597,7 @@ void ofNode::orbitDeg(float longitude, float latitude, float radius, const glm::
p = q * p; // rotate p on unit sphere based on quaternion
p = p * radius; // scale p by radius from its position on unit sphere

setGlobalPosition(centerPoint + p);
setGlobalPosition(centerPoint + p.xyz());
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, an intermediate ofVec3f conversion was being used.

@@ -132,9 +132,9 @@ void ofNode::setParent(ofNode& parent, bool bMaintainGlobalTransform) {
clearParent(bMaintainGlobalTransform);
}
if(bMaintainGlobalTransform) {
auto postParentPosition = position - parent.getGlobalPosition();
auto postParentPosition = position.get() - parent.getGlobalPosition();
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, an intermediate ofVec3f conversion was being used.

@@ -756,7 +756,7 @@ void ofAppGLFWWindow::setFullscreen(bool fullscreen){
GLFWmonitor** monitors = glfwGetMonitors(&monitorCount);

int currentMonitor = getCurrentMonitor();
ofVec3f screenSize = getScreenSize();
Copy link
Member Author

Choose a reason for hiding this comment

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

Leftover ofVec.

@@ -1138,14 +1138,14 @@ void ofVertices( const vector <glm::vec2> & polyPoints ){
//----------------------------------------------------------
void ofVertices( const vector <ofVec3f> & polyPoints ){
for( const auto & p: polyPoints ){
ofGetCurrentRenderer()->getPath().lineTo(p);
ofGetCurrentRenderer()->getPath().lineTo(glm::vec3(p.x, p.y, p.z));
Copy link
Member Author

Choose a reason for hiding this comment

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

Use explicit conversions throughout.

@@ -42,11 +42,11 @@ class ofMatrix3x3 {
float _g=0.0, float _h=0.0, float _i=0.0 );


ofMatrix3x3( const glm::mat3 & mat) {
OF_DEPRECATED_MSG("Use glm::mat3.", ofMatrix3x3( const glm::mat3 & mat)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Encourage the use of glm::* without breaking things.

@@ -243,88 +243,88 @@ namespace glm {

//--------------------------------------------------------------
inline glm::vec3 operator+(const glm::vec3 & v1, const ofVec3f & v2){
return v1 + glm::vec3(v2);
return v1 + glm::vec3(v2.x, v2.y, v2.z);
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove implicit conversions.

bakercp and others added 19 commits May 4, 2017 22:20
Switched from string to filesystem::path for some utility functions.
+ ofMatrixStack depends on ofFbo, although it really only calls methods
  of ofFbo's base class, ofBaseDraws - essentially queries onto the
  RenderSurface's geometry.

The proposed changes update ofMatrixStack to require just an
ofBaseDraws compatible object to be passed as target render surface.

This allows other renderers, such as the vulkan renderer, to use this
object, without having access to ofFbo.
…ix_stack_from_fbo

remove dependency ofMatrixStack<->ofFbo
…ists

Making sure json file exists before reading from it
…erial

Fix warning about unused function.
ofSerial: fix port name vs. path parsing on Windows
bakercp and others added 4 commits May 18, 2017 23:20
@arturoc
Copy link
Member

arturoc commented May 20, 2017

i'm not so sure about this, people are going to use ofVec for a long while and although i see how it's really useful for catching where you are still using ofVec i think it can also be a problem cause a lot of addons are using ofVec and most people are not going to port them right away (if at all) so this is going to throw a million warnings in most projects

even most examples still use ofVec right now

perhaps there could be a flag to enable this? this is only header code so you can just do:

#define OF_DEPRECATE_OFVEC
#include "ofMain.h"

or define that in the project files, makefiles... while compiling the core, examples... so we can catch usage of ofVec

@bakercp
Copy link
Member Author

bakercp commented May 20, 2017

Well, the intention was to "encourage" people to migrate via deprecation warnings since we are deprecating ofVec. Deprecation warnings are indeed annoying, but that is their purpose no? Keeping oF in limbo for more than a CV release or two is going to be a bit of a maintenance pain. Most people probably won't read the documentation, but the will see the warnings :)

@bakercp
Copy link
Member Author

bakercp commented Sep 7, 2017

@arturoc Any thoughts on this?

@bakercp bakercp self-assigned this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants